Codecov Report

Merging #1051 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
+ Coverage   97.24%   97.25%   +<.01%     
==========================================
  Files          90       90              
  Lines        6981     6988       +7     
==========================================
+ Hits         6789     6796       +7     
  Misses        192      192

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

Comments from Reviewable

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

ycmd/responses.py, line 248 at r1 (raw file):

    location = Location( 1, 1, filename )
    location_extent = Range( location, location )
    diagnostics.append( Diagnostic(

I don't think it is a problem, and certainly no need to change it, but there is a slight irony that in the case that we exceed the max number of diags, we send exactly the max number of diags plus one diagnostics.

ycmd/completers/cpp/clang_completer.py, line 361 at r1 (raw file):

    self._diagnostic_store = DiagnosticsToDiagStructure( diagnostics )
    return responses.BuildDiagnosticResponse( diagnostics,
                                              filename,

hmm in this case, the special extra diagnostic is associated with the file name of the TU. In the case of a huge unity build, this might not be the file the user is editing. I feel like we should be sticking this diagnostic on the request_data[ 'filepath' ] so that it is prominent for the user?

ycmd/completers/language_server/language_server_completer.py, line 643 at r1 (raw file):

    super( LanguageServerCompleter, self ).__init__( user_options )

    self._max_diagnostics_to_display = user_options[

we might consider hoisting this up into the Completer base class? It's now appearing in a number of places.

ycmd/tests/language_server/language_server_completer_test.py, line 456 at r1 (raw file):

def LanguageServerCompleter_Diagnostics_MaxDiagnosticsNumberExceeded_test():
  completer = MockCompleter( { 'max_diagnostics_to_display': 1 } )

what was the reason to use a mock here, rather than requesting the actual diagnostics? I was curious to see how this affected diags across multiple files as reported by jdt.ls. I realise that the diagnostics tests for java are quite fiddly.

Comments from Reviewable

Reviewed 15 of 15 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

with a couple observations

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

Comments from Reviewable

Reviewed 15 of 15 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

ycmd/responses.py, line 248 at r1 (raw file):

Yes, I thought about it and I know that Clang handles this by removing a diagnostic but I think that, while technically correct, it's not intuitive e.g. the limit is 30 and there are 31 errors but only 29 of the 30 expected errors appear?

ycmd/completers/cpp/clang_completer.py, line 361 at r1 (raw file):

I completely forgot about the TU thing. Thanks for catching this. Fixed.

ycmd/completers/language_server/language_server_completer.py, line 643 at r1 (raw file):

Makes sense. Done.

ycmd/tests/language_server/language_server_completer_test.py, line 456 at r1 (raw file):

I didn't add tests to the Java completer because I don't want to make the CI builds even more flaky.

Comments from Reviewable

Review status: 0 of 2 LGTMs obtained (and 2 stale)

ycmd/completers/cpp/clang_completer.py, line 361 at r1 (raw file):

:D its my pet feature :)

ycmd/tests/language_server/language_server_completer_test.py, line 456 at r1 (raw file):

Makes sense :/

Comments from Reviewable

@zzbot r+

Review status: 1 of 2 LGTMs obtained (and 1 stale)

Comments from Reviewable

📌 Commit c9db1f4 has been approved by micbou

⌛ Testing commit c9db1f4 with merge 0126e99...

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 0126e99 to master...