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

GetHoverResponse should handle null response from server. #1110

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

kadircet
Copy link
Contributor

@kadircet kadircet commented Sep 26, 2018

As mentioned in Hover Request

Response:

result: Hover | null

Current implementation was failing with None object has no getitem method, with this patch it will provide more useful information.


This change is Reviewable

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make a test for the case where the response to the hover request is null? You can take a look at ycmd/tests/language_server/ to get an idea how those are written.
However, all hover tests are currently written against the java server, which never returns null, hence why we currently don't handle null response. Anyway, ask for help if you are stuck.

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 2 LGTMs obtained

@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #1110 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
- Coverage   97.59%   97.58%   -0.02%     
==========================================
  Files          89       89              
  Lines        6988     6992       +4     
==========================================
+ Hits         6820     6823       +3     
- Misses        168      169       +1

@kadircet
Copy link
Contributor Author

Added tests for GetHoverResponse, both for null and non-null case.

Copy link
Collaborator

@micbou micbou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: except a minor comment.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)


ycmd/completers/language_server/language_server_completer.py, line 1311 at r2 (raw file):

      return result[ 'contents' ]
    raise RuntimeError( 'No hover information.' )

Unnecessary blank line.

Copy link
Collaborator

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also left a minor comment. Squashing the commits into one would also be appreciated.
Apart from that, :lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale)


ycmd/tests/language_server/language_server_completer_test.py, line 621 at r2 (raw file):

                       side_effect = [ { 'result': { 'contents': 'test' } } ] ):
      assert_that(
        completer.GetHoverResponse( request_data ), equal_to( 'test' )

Nose provides eq_ so this whole assert can be fit on one line.

from nose.tools import eq_
eq_(actual, expected)

Also the raises( RuntimeError ) can take an optional parameter to match the exception string. For example raises( RuntimeError, 'No hover information' ). It might also be good to extract that string to a variable like it is done in ycmd/tests/bindings/cpp_bindings_raises_exception_test.py.

@kadircet kadircet force-pushed the gethover_handle_null branch from c567755 to e1a6e18 Compare September 27, 2018 13:04
@kadircet
Copy link
Contributor Author

Addressed all the comments and squashed commits.

@bstaletic
Copy link
Collaborator

Thanks for the PR.
@zzbot r+

@zzbot
Copy link
Contributor

zzbot commented Sep 27, 2018

📌 Commit e1a6e18 has been approved by bstaletic

@zzbot
Copy link
Contributor

zzbot commented Sep 27, 2018

⌛ Testing commit e1a6e18 with merge 50a358b...

zzbot added a commit that referenced this pull request Sep 27, 2018
GetHoverResponse should handle null response from server.

As mentioned in [Hover Request](https://microsoft.github.io/language-server-protocol/specification#textDocument_hover)
```
Response:

result: Hover | null
```
Current implementation was failing with None object has no _getitem_ method, with this patch it will provide more useful information.

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

zzbot commented Sep 27, 2018

💔 Test failed - status-travis

@micbou
Copy link
Collaborator

micbou commented Sep 27, 2018

@zzbot retry

@zzbot
Copy link
Contributor

zzbot commented Sep 27, 2018

⌛ Testing commit e1a6e18 with merge ad978f4...

zzbot added a commit that referenced this pull request Sep 27, 2018
GetHoverResponse should handle null response from server.

As mentioned in [Hover Request](https://microsoft.github.io/language-server-protocol/specification#textDocument_hover)
```
Response:

result: Hover | null
```
Current implementation was failing with None object has no _getitem_ method, with this patch it will provide more useful information.

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

zzbot commented Sep 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: bstaletic
Pushing ad978f4 to master...

@zzbot zzbot merged commit e1a6e18 into ycm-core:master Sep 28, 2018
zzbot added a commit to ycm-core/YouCompleteMe that referenced this pull request Oct 7, 2018
[READY] Update ycmd

Include the following changes:
 - PR ycm-core/ycmd#1075: remove Node.js workaround for Debian-based distributions;
 - PR ycm-core/ycmd#1077: update Boost to 1.68;
 - PR ycm-core/ycmd#1078: add semantic trigger for Julia;
 - PR ycm-core/ycmd#1081: improve type information on C++ member functions;
 - PR ycm-core/ycmd#1086: check if Python headers are installed before building;
 - PR ycm-core/ycmd#1088: raise proper exception for commands when file is still being parsed in C-family languages;
 - PR ycm-core/ycmd#1098: update Go completer;
 - PR ycm-core/ycmd#1099: update regex submodule;
 - PR ycm-core/ycmd#1100: add Python path to debugging information;
 - PR ycm-core/ycmd#1103: support framework headers completion and code navigation in C-family languages;
 - PR ycm-core/ycmd#1107: update Clang to 7.0.0;
 - PR ycm-core/ycmd#1109: update jdt.ls to 0.25.0;
 - PR ycm-core/ycmd#1110: handle null hover response from language servers;
 - PR ycm-core/ycmd#1111: update list of completion kinds defined by LSP;
 - PR ycm-core/ycmd#1113: send completion item kinds capability to language servers;
 - PR ycm-core/ycmd#1116: update Parso to 0.3.1 and Jedi to 0.13.1.

Closes #3138.
Closes #3145.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/3175)
<!-- Reviewable:end -->
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.

4 participants