[READY] Print diagnostic code field #1239

kadircet · 2019-04-26T13:49:48Z

Clangd started populating diagnostic's code field with relevant information with https://reviews.llvm.org/D60822 and https://reviews.llvm.org/D58291.

This patch aims to surface that information to ycmd users, as vscode does.

This change is

bstaletic

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

micbou

Thanks for the PR. Looks good but I think it would be better to extend our diagnostic API with a code field and do the formatting on the client side.

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

bstaletic

@micbou Does that really matter?

Reviewable status: 1 of 2 LGTMs obtained

kadircet

I don't think clients would benefit more in that case. Because I would expect all of them to simply append this to diag text. In addition to that it would also require clients to be modified as well, which makes things a little bit more complicated.

Reviewable status: 1 of 2 LGTMs obtained

puremourning

I tend to agree that just putting it in the text is fine. We have filtering etc. that applies only to the text.

However, it's not obvious to me why we're only doing this for non-floating-point errors. Wouldn't it be simpler to just always include it?

Reviewable status: 1 of 2 LGTMs obtained

puremourning

Reviewed 2 of 2 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @kadircet)

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

      float( code )
    except ValueError:
      # We are only interested in non-number codes.

why is that?

bstaletic

Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

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

Previously, puremourning (Ben Jackson) wrote…

why is that?

IIRC, this is supposed to be presented to users, something like appending [-Wwarning-name] to a diagnostic. If the name is some sort of number, because the server (stupidly) exposes internal IDs, we would end up appending [123]. I can hear the confusion of the users going "WAT?".

In short, using numbers just adds verbosity, but doesn't provide any useful information.

puremourning

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

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

Previously, bstaletic (Boris Staletic) wrote…

IIRC, this is supposed to be presented to users, something like appending [-Wwarning-name] to a diagnostic. If the name is some sort of number, because the server (stupidly) exposes internal IDs, we would end up appending [123]. I can hear the confusion of the users going "WAT?".

In short, using numbers just adds verbosity, but doesn't provide any useful information.

unless the particular compiler/language uses numbers to represent error codes. I've seen plenty of those. Anyway i suppose we could revisit that if necessary.

Checking the specification, the type of 'message' is 'number | string', so this check rather than being "convertible" to string, should be "is-a" string, i.e. isinstance( code, str )

bstaletic

Reviewable status: 1 of 2 LGTMs obtained (waiting on @kadircet)

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

Previously, puremourning (Ben Jackson) wrote…

unless the particular compiler/language uses numbers to represent error codes. I've seen plenty of those. Anyway i suppose we could revisit that if necessary.

Checking the specification, the type of 'message' is 'number | string', so this check rather than being "convertible" to string, should be "is-a" string, i.e. isinstance( code, str )

I also believe @kadircet said JDT sends something like {"code": "123"}, thus sending numbers as strings.

kadircet

Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

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

Previously, bstaletic (Boris Staletic) wrote…

I also believe @kadircet said JDT sends something like {"code": "123"}, thus sending numbers as strings.

yes, IIRC there were breakages inside jdt.ls tests, since they were representing numbers with strings.

puremourning

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic and @kadircet)

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

Previously, kadircet (kadir çetinkaya) wrote…

yes, IIRC there were breakages inside jdt.ls tests, since they were representing numbers with strings.

Are we sure that the numbers are not useful/meaningful to java developers ? Would users not want to use those numbers for filtering ?

kadircet

they looked arbitrary with 8+ digits, I am not sure anyone has that good memory :D
I just thought it might annoy users to see those digits, but it is totally ok for me to surface them as well

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic and @kadircet)

puremourning

OK well let's go with this and we can expand later if there's a request to.

Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @bstaletic and @kadircet)

puremourning

Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @bstaletic and @kadircet)

puremourning · 2019-06-21T13:01:49Z

@kadircet can you rebase and push the PR ? It still has the old CI system on the commit checks !

codecov · 2019-06-21T13:04:32Z

Codecov Report

Merging #1239 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
+ Coverage   97.14%   97.18%   +0.04%     
==========================================
  Files          95       96       +1     
  Lines        7343     7243     -100     
  Branches      153        0     -153     
==========================================
- Hits         7133     7039      -94     
- Misses        169      204      +35     
+ Partials       41        0      -41

codecov · 2019-06-21T13:04:32Z

Codecov Report

Merging #1239 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
+ Coverage   97.14%   97.18%   +0.04%     
==========================================
  Files          95       96       +1     
  Lines        7343     7243     -100     
  Branches      153        0     -153     
==========================================
- Hits         7133     7039      -94     
- Misses        169      204      +35     
+ Partials       41        0      -41

puremourning · 2019-06-21T13:32:26Z

Tests are failing on RLS

kadircet · 2019-06-21T13:35:31Z

yup just saw. I believe rust was not merged when I've written the patch?

They appear to be reporting codes like 'E0609', I have no idea whether they are meaningful or not. Anyone knows if these are worth keeping ?

bstaletic

@theli-ua Can you make sense of diagnostic code E0609?

Reviewed 2 of 2 files at r2.
Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @puremourning)

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

Previously, puremourning (Ben Jackson) wrote…

Are we sure that the numbers are not useful/meaningful to java developers ? Would users not want to use those numbers for filtering ?

Filtering! That might be a good reason to just flat out pass anything, even [123].

puremourning

Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @bstaletic and @kadircet)

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

Previously, bstaletic (Boris Staletic) wrote…

Filtering! That might be a good reason to just flat out pass anything, even [123].

Yeah.. that was my point :)

bstaletic

Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @puremourning)

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

Previously, puremourning (Ben Jackson) wrote…

Yeah.. that was my point :)

@kadircet Is there any problem with just always adding the code to the diagnostic? It would make it much easier for users to filter diagnostics if they need to and it would fix the tests.

codecov-io · 2019-06-26T12:47:14Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f16d443). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1239   +/-   ##
=========================================
  Coverage          ?   97.18%           
=========================================
  Files             ?       96           
  Lines             ?     7243           
  Branches          ?        0           
=========================================
  Hits              ?     7039           
  Misses            ?      204           
  Partials          ?        0

kadircet

Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @bstaletic and @puremourning)

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

Previously, bstaletic (Boris Staletic) wrote…

@kadircet Is there any problem with just always adding the code to the diagnostic? It would make it much easier for users to filter diagnostics if they need to and it would fix the tests.

Done

bstaletic

If tests fail, a rebase will be necessary, but otherwise

Reviewed 3 of 3 files at r3.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

bstaletic

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @kadircet and @puremourning)

ycmd/completers/language_server/language_server_completer.py, line 2244 at r3 (raw file):

  try:
    code = diag[ 'code' ]
    diag_text += " [" + code + "]"

code needs to be str( code )

kadircet

Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @puremourning)

ycmd/completers/language_server/language_server_completer.py, line 2244 at r3 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

code needs to be str( code )

Done.

puremourning · 2019-06-26T15:41:16Z

Latest master should be more flake-free. So maybe one more rebase ?

puremourning

Reviewed 1 of 3 files at r3, 3 of 3 files at r4.
Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @bstaletic)

puremourning

Reviewed 2 of 2 files at r5.
Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @bstaletic)

mergify · 2019-06-26T17:38:29Z

Thanks for sending a PR!

mergify · 2019-06-26T17:38:30Z

Thanks for sending a PR!

ycm-core/ycmd#1275 Remap python GoTo* commands ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn ycm-core/ycmd#1239 Error ID in diagnostic messages ycm-core/ycmd#1267 Clangd extra conf support ycm-core/ycmd#1266 JDT update to 0.40.0 ycm-core/ycmd#1245 Generic (pluggable) LSP completer support ycm-core/ycmd#1263 Support JDT extensions ycm-core/ycmd#1261 Typescript update ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration ycm-core/ycmd#1262 Multi-user installation ycm-core/ycmd#1243 Migrate to GOPLS ycm-core/ycmd#1249 Support MSVC 16 (VS2019) ycm-core/ycmd#1224 Migrate to RLS

bstaletic approved these changes Apr 26, 2019

View reviewed changes

micbou requested changes Apr 26, 2019

View reviewed changes

bstaletic approved these changes Apr 26, 2019

View reviewed changes

kadircet commented Apr 29, 2019

View reviewed changes

puremourning approved these changes Jun 21, 2019

View reviewed changes

puremourning requested changes Jun 21, 2019

View reviewed changes

bstaletic approved these changes Jun 21, 2019

View reviewed changes

puremourning requested changes Jun 21, 2019

View reviewed changes

bstaletic approved these changes Jun 21, 2019

View reviewed changes

kadircet commented Jun 21, 2019

View reviewed changes

puremourning requested changes Jun 21, 2019

View reviewed changes

kadircet commented Jun 21, 2019

View reviewed changes

puremourning reviewed Jun 21, 2019

View reviewed changes

puremourning approved these changes Jun 21, 2019

View reviewed changes

kadircet force-pushed the clangd_diag_code branch from d77b729 to 7ad9f3e Compare June 21, 2019 13:04

bstaletic approved these changes Jun 21, 2019

View reviewed changes

puremourning requested changes Jun 21, 2019

View reviewed changes

bstaletic approved these changes Jun 26, 2019

View reviewed changes

kadircet force-pushed the clangd_diag_code branch from 7ad9f3e to a7473f7 Compare June 26, 2019 12:47

kadircet commented Jun 26, 2019

View reviewed changes

bstaletic approved these changes Jun 26, 2019

View reviewed changes

bstaletic requested changes Jun 26, 2019

View reviewed changes

kadircet force-pushed the clangd_diag_code branch from a7473f7 to 0c09ae0 Compare June 26, 2019 13:26

kadircet commented Jun 26, 2019

View reviewed changes

kadircet changed the title ~~Print diagnostic code field if it is non-number~~ [READY] Print diagnostic code field Jun 26, 2019

puremourning approved these changes Jun 26, 2019

View reviewed changes

Print diagnostic code

328f023

kadircet force-pushed the clangd_diag_code branch from 0c09ae0 to 328f023 Compare June 26, 2019 16:08

puremourning approved these changes Jun 26, 2019

View reviewed changes

mergify bot merged commit 6eef083 into ycm-core:master Jun 26, 2019

kadircet deleted the clangd_diag_code branch June 27, 2019 08:38

bstaletic mentioned this pull request Jul 4, 2019

[READY] Update ycmd submodule ycm-core/YouCompleteMe#3432

Merged

4 tasks

[READY] Print diagnostic code field #1239

[READY] Print diagnostic code field #1239

Uh oh!

Conversation

kadircet commented Apr 26, 2019 • edited by Valloric Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

micbou left a comment

Choose a reason for hiding this comment

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning left a comment

Choose a reason for hiding this comment

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning left a comment

Choose a reason for hiding this comment

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning left a comment

Choose a reason for hiding this comment

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning commented Jun 21, 2019

Uh oh!

codecov bot commented Jun 21, 2019

Codecov Report

Uh oh!

codecov bot commented Jun 21, 2019

Codecov Report

Uh oh!

puremourning commented Jun 21, 2019

Uh oh!

kadircet commented Jun 21, 2019

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

puremourning left a comment

Choose a reason for hiding this comment

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

kadircet commented Apr 26, 2019 •

edited by Valloric

Loading

codecov-io commented Jun 26, 2019 •

edited

Loading