HTTP responses#20
Conversation
| @@ -1,5 +1,7 @@ | |||
| #if (UNITY_EDITOR) | |||
There was a problem hiding this comment.
It should be on top, for beauty 🍃
| var heartbeat = new Heartbeat(file, fromSave); | ||
| if ((heartbeat.time - _lastHeartbeat.time < HEARTBEAT_COOLDOWN) && !fromSave && | ||
| if ((heartbeat.time - _lastHeartbeat.time < HEARTBEAT_COOLDOWN) && !fromSave && | ||
| //BUG: _lastHeartbeat has no time, entity or type |
There was a problem hiding this comment.
It's a good practice to not intervent long conditionals (even multi-line) with comments, put it on top instead.
| request.SendWebRequest().completed += | ||
| operation => { | ||
| if (request.downloadHandler.text == string.Empty) { | ||
| /*if (request.downloadHandler.text == string.Empty) { |
There was a problem hiding this comment.
Should put a comment on top of the block explaining why is it commented out.
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks for status code and takes correspond actions |
There was a problem hiding this comment.
| /// Checks for status code and takes correspond actions | |
| /// Checks for status code and then acts accordingly |
|
I'm sorry for the delay, @Hermesiss, I'm a bit busy with messy containers security! First of all, thanks for your time. I do appreciate your help on the project. I guess this PR is aimed to suppress malformed Wakatime responses. That's good, I think, as it's a third-party bug and we don't want to disrupt the user experience. A small note — it's generally bad practice to include commented out blocks of code into the master branch. We always have the history, so we could add it back once it's working as intended. I think the |
|
I may be in some way out of understanding the real purpose of this PR. Which issue particularly does it fix? As per #13, the problem was malformed response, which is now solved. In everyday programming (especially for such small projects as this) we should rely on third party API to work as expected most of the time, handling all errors in bulk with minimum code. This reduces the complexity and therefore improves maintainability. In this case, expected error is 429 only, all other are unexpected and should throw. There is no need in this |
|
Additionally, for better communication, you should split the PRs as much as possible, as it's sometimes hard to follow their purpose. Single PR = single change (or complex overhaul). |
|
There are 3 important for us error codes that require special reactions: While working on In this PR I want to move scattered data checks to one place. I took all possible codes from dev section. |
2 participants
You can review existing code while we are waiting for https://github.com/wakatime/wakatime/issues/181