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

Display diagnostic error codes #49215

Closed
tboby opened this issue May 4, 2018 · 21 comments
Closed

Display diagnostic error codes #49215

tboby opened this issue May 4, 2018 · 21 comments
Assignees
Labels
error-list Problems view feature-request Request for new features or functionality languages-diagnostics Source problems reporting verification-needed Verification of issue is requested verified Verification succeeded

Comments

@tboby
Copy link

tboby commented May 4, 2018

Would it be possible for the diagnostic error code to be surfaced to the user in some way?

Diagnostics as defined in the LSP and vscode-api have a code field which is not easily visible to the user in vscode. Language servers/compilers often allow the suppression of specific diagnostics by passing a list of error codes to ignore. Currently, as far as I know, extensions have to intercept LSP diagnostics and hardbake the code into error message, or incorrectly use the source field.

E.g:
image
image
Both of these solutions are less than ideal, as they mean that the raw "Copy" data is incorrect, and inconsistent across extensions.

It seems odd that source, a field that is generally obvious from context, is displayed, but there is no proper support for the code field.

I'm not sure what the best way of actually displaying it would be, but a few things come to mind:

  1. Add it to the hover (as suggested by @egamma in Problems/Diagnostics does not show "Code" #44017 )
  2. Display it together with the source
  3. Display it instead of the source, and move the source to the end of the row
@vscodebot
Copy link

vscodebot bot commented May 4, 2018

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

@tboby
Copy link
Author

tboby commented May 4, 2018

Neither of these issues are duplicate, although the second isn't far off, nice try!

@jrieken jrieken added feature-request Request for new features or functionality languages-diagnostics Source problems reporting labels May 4, 2018
@jrieken
Copy link
Member

jrieken commented May 4, 2018

Yes, fair request. For the sake of completeness there is the hover, the f8-feature, and the error list.

It seems odd that source, a field that is generally obvious from context, is displayed,

The motivation for that is the presence of different validators, e.g. TypeScript for syntax/semantic checks and TSLint for linting.

Adding @sandy081 for the errors list and for more feedback

@sandy081
Copy link
Member

sandy081 commented May 7, 2018

I think the current format [${source}] ${message} (${line}, ${column}) is simple and clean. If we want to add code I would prefer to keep it before message, just like the example 1 in the description:

[${source}] ${code}: ${message} (${line}, ${column}) ??

@egamma
Copy link
Member

egamma commented May 7, 2018

How about showing it at the end. The message is the most important part of a diagnostic and it would be good when it comes first.

[${source}]: ${message} (${line}, ${column}) ${code} ??

@mattacosta
Copy link
Contributor

I'm also guilty of (mis)using the source field for error codes.

While I also agree that the message is the most important part, I would also say that the code is equal to or greater in importance than the source because it is useful for things like error suppression comments or looking up documentation (such as googling "c# error 1234"), while the source is only useful if someone has multiple extensions with non-obvious diagnostics. Therefore option 3 would seem to be the best fit, but option 2 would also be acceptable if it's too much of a change.

@sandy081 sandy081 added the error-list Problems view label Aug 13, 2018
@sandy081 sandy081 added this to the Backlog milestone Aug 13, 2018
@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Aug 13, 2018

The C/C++ extension is adding error/warning codes so that users can disable particular errors/warnings like VS 2017 has so we'd highly like VS Code to add this. In the mean time, how do you recommend we expose the error codes to users? Our current plan is to add it to the end of the message until this is fixed. We're also adding "C/C++" as the source and many users have requested filtering based on that (I think there's another issue tracking that).

@sandy081 sandy081 modified the milestones: Backlog, September 2018 Sep 20, 2018
@sandy081
Copy link
Member

How about showing error code at the end as @egamma mentioned

image

@jrieken
Copy link
Member

jrieken commented Sep 20, 2018

Maybe not at the end-end but before the line/column info?

@tboby
Copy link
Author

tboby commented Sep 20, 2018

Oh, please prioritise it over message on narrow screens (like line numbers currently do)
image

Also, minor point, having line/column as far right causes things to line up nicely!

@sandy081
Copy link
Member

Before line numbers

image

@tboby

image

sandy081 added a commit that referenced this issue Sep 20, 2018
@sandy081 sandy081 added the verification-needed Verification of issue is requested label Sep 20, 2018
@jrieken jrieken added the verified Verification succeeded label Sep 25, 2018
@sandy081
Copy link
Member

@egamma I see diagnostic code is being repeated in message part for TS Lint diagnostics. Is it possible to remove them from the message?

@egamma
Copy link
Member

egamma commented Oct 10, 2018

@sandy081 I´ve removed rendering of the code (the tslint ruleName) for tslint, but now the rule name no longer shows up in the hover. So the question is whether we should also show the code in the hover somehow.

image

@egamma
Copy link
Member

egamma commented Oct 10, 2018

@dbaeumer pls check whether you have the same issue of duplicate error code in eslint.

@cdeutsch
Copy link

cdeutsch commented Oct 10, 2018

Please bring back the name in the hover. If I need to disable a rule that's where I get the name from.

Now I'm stuck googling the error message or linting the whole project from the command line to get the name 🤦‍♂️

@dbaeumer
Copy link
Member

@egamma I do.

@sandy081 the recommendation is to remove it right and we will add the code to the hover automatically as well.

@sandy081
Copy link
Member

Yes

@dbaeumer
Copy link
Member

Removed it from ESLint and published new version for ^1.28

@drKnoxy
Copy link

drKnoxy commented Oct 18, 2018

Hey i'm the maintainer of eslint-disable, any tips on how to get at the rule id so that we can disable rules again?

@dbaeumer
Copy link
Member

@drKnoxy you might want to have a look at microsoft/vscode-eslint#530 which adds the as quick fixes to the extension directly.

@charpeni
Copy link

charpeni commented Nov 1, 2018

Starting from vscode-eslint@1.6.1, the easiest way to get eslint's rule id is to open the Problems window.

image

cc @cdeutsch @drKnoxy

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-list Problems view feature-request Request for new features or functionality languages-diagnostics Source problems reporting verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

10 participants