Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[READY] Use equality instead of identity when checking the status response in Rust completer #521

Merged
merged 2 commits into from
Jun 10, 2016

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Jun 9, 2016

http.client.NO_CONTENT is an enumeration member while response.status_code is an int so we need to use an equality instead of an identity.

Added a test for when racerd returns a no content response.

On a side note, I think we should use requests.codes instead of http.client in these situations. This would prevent this kind of issue (requests.codes.no_content is an int) and we would not have to import the http.client module. If you agree, I'll send a PR replacing all http.client occurences.


This change is Reviewable

Check that no error is raised and completions are empty when racerd
returns a no content response.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 85.082% when pulling e681335 on micbou:rust-completer-fix-no-content-response into b779b68 on Valloric:master.

@vheon
Copy link
Contributor

vheon commented Jun 9, 2016

:lgtm:

On a side note, I think we should use requests.codes instead of http.client in these situations.

I agree with this.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@vheon
Copy link
Contributor

vheon commented Jun 9, 2016

Either edit the commit message to close #520 or remember to close it by hand 😃

@micbou
Copy link
Collaborator Author

micbou commented Jun 9, 2016

Unfortunately, it doesn't fix the issue. In #520, racerd is crashing instead of returning a 204 no content response.

Previously, vheon (Andrea Cedraro) wrote…

Either edit the commit message to close #520 or remember to close it by hand 😃


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@vheon
Copy link
Contributor

vheon commented Jun 9, 2016

Unfortunately, it doesn't fix the issue. In #520, racerd is crashing instead of returning a 204 no content response.

The user that opened the bug said that racerd didn't crash so I assumed that this was going to fix the bug on our hand but I should've read more carefully 😒


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@Valloric
Copy link
Member

The real problem here was the lack of test coverage for the NO_CONTENT branch.

Thanks for fixing this!

:lgtm:

Previously, vheon (Andrea Cedraro) wrote…

Unfortunately, it doesn't fix the issue. In #520, racerd is crashing instead of returning a 204 no content response.

The user that opened the bug said that racerd didn't crash so I assumed that this was going to fix the bug on our hand but I should've read more carefully 😒


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@micbou
Copy link
Collaborator Author

micbou commented Jun 10, 2016

@homu r+

@Valloric You didn't give your opinion about using requests.codes instead of http.client for HTTP status codes. What do you think?

Previously, Valloric (Val Markovic) wrote…

The real problem here was the lack of test coverage for the NO_CONTENT branch.

Thanks for fixing this!

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@homu
Copy link
Contributor

homu commented Jun 10, 2016

📌 Commit e681335 has been approved by micbou

@homu
Copy link
Contributor

homu commented Jun 10, 2016

⚡ Test exempted - status

@homu homu merged commit e681335 into ycm-core:master Jun 10, 2016
homu added a commit that referenced this pull request Jun 10, 2016
…=micbou

[READY] Use equality instead of identity when checking the status response in Rust completer

`http.client.NO_CONTENT` is an enumeration member while `response.status_code` is an int so we need to use an equality instead of an identity.

Added a test for when racerd returns a no content response.

On a side note, I think we should use [`requests.codes` instead of `http.client`](http://docs.python-requests.org/en/master/user/quickstart/?highlight=status_code#response-status-codes) in these situations. This would prevent this kind of issue (`requests.codes.no_content` is an int) and we would not have to import the `http.client` module. If you agree, I'll send a PR replacing all `http.client` occurences.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/521)
<!-- Reviewable:end -->
@Valloric
Copy link
Member

@Valloric You didn't give your opinion about using requests.codes instead of http.client for HTTP status codes. What do you think?

Sorry, missed that. I'm fine with it.

@micbou micbou deleted the rust-completer-fix-no-content-response branch June 12, 2016 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants