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

Introduce option to prevent underlining multiple lines #463

Closed
wants to merge 1 commit into from

Conversation

bsuh
Copy link
Contributor

@bsuh bsuh commented May 7, 2018

Fixes #370

Demo
singelineunderline

@msftclas
Copy link

msftclas commented May 7, 2018

CLA assistant check
All CLA requirements met.

@dbaeumer
Copy link
Member

dbaeumer commented May 7, 2018

@bsuh thanks for the PR.

Actually I am pretty reluctant to add something like this even if it is behind a flag. I always stayed away from tempering with eslint error information. This might very likely lead to other problems. For example when running an ESLint task which will report a multi line error again which you can't temper with.

Why do you want to have this feature.

In addition such a setting must be a resource setting and not a global setting. It will avoid the need for reloading the window when the setting changes.

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label May 7, 2018
@bsuh
Copy link
Contributor Author

bsuh commented May 7, 2018

The scope is set as "resource". I am evaluating switching editors from emacs to VSCode, so I am not familiar at all with VSCode or writing extensions. I couldn't get the setting to take effect, because the configuration is passed during server initialization. How can I pass the new configuration value to the server?

I want this feature because I have to maintain a brownfield project that doesn't use default exports and my team decided to adopt a very opinionated ESLint configuration, with one of the rules being eslint-import/prefer-default-export, the one in the demo that underlines the entire object / class masking pretty much every other ESLint error / warning within the object / class.

@dbaeumer
Copy link
Member

dbaeumer commented May 8, 2018

Resource level operations are resolved here: https://github.com/Microsoft/vscode-eslint/blob/master/client/src/extension.ts#L477

I still think tempering with error informatin caused more problems down the road even if behind a flag.

@bsuh
Copy link
Contributor Author

bsuh commented May 8, 2018

Ok. Can you give actionable advice whether that is "Stop working on the PR, it won't be merged" or "Add a warning to the description of the setting"?

@dbaeumer dbaeumer mentioned this pull request May 30, 2018
@dbaeumer
Copy link
Member

I do see the problems with the huge red underlines. Since this extension has now knowledge about the AST it can only shorten the underline to one line. A better solution might be to have the underline only on a meaningful AST node (e.g. Something). But something like that has to come from the ESLint module itself. The advantage of having it there would be:

  • consistent since it would print the same error message independent whether eslint is run as an extension, task, in the terminal, ...
  • something more meaningful can be underlined that the whole first line

Have you tried to talk to the ESLint people about this to see what they think. It could be a general option there.

@bsuh
Copy link
Contributor Author

bsuh commented Jun 5, 2018

eslint/eslint#10424 (comment)

Seems like ESLint are leaning against adding such a thing.

@aronfiechter
Copy link

Are there any news about this?
The people on ESLint are against this because they prefer to report all the information in a consistent way, and this makes sense since ESLint is first and foremost a tool, and does not necessarily have something to do with a GUI.

Instead, vscode-eslint is definitely about GUI, and having an entire file or class or function underlined in red is not useful, since it masks all other errors inside said file/class/function.

I'd even suggest having the ability to just show the error on the first token in the range, not even the entire single line, so to not cover subsequent errors in the same line.

What is the argument against having this as an option? Without this option the extension is pretty much useless unless I use a very outdated version of eslint.

Base automatically changed from master to main March 2, 2021 14:45
@dbaeumer
Copy link
Member

dbaeumer commented Oct 8, 2021

I will close the PR since I am still not a fan to add this due to consistency

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info-needed Issue requires more information from poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Underlining entire object on eslint error
4 participants