Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

linter-eslint is very loud when stuff is not installed #387

Closed
dominicbarnes opened this issue Dec 24, 2015 · 17 comments
Closed

linter-eslint is very loud when stuff is not installed #387

dominicbarnes opened this issue Dec 24, 2015 · 17 comments
Assignees

Comments

@dominicbarnes
Copy link

Currently, if linter-eslint tries to run, but stuff is missing from npm, it complains very loudly and repeatedly as I try to navigate through the project. It is really obstructive, and it quickly gets to the point where I either have to disable linter-eslint (which is a pain that I sometimes forget to undo) or I have to wait on npm install. (sometimes it just takes longer than I want)

One idea I have: if the exact same error message comes up repeatedly, it would only be sent as a notification one time. This could be accomplished by using err.message as a key and setting a timeout of 5 minutes or something before that message will be allowed to be shown again. (with the timeout length configurable)

To be clear, this is definitely a nit-pick on my part, especially since the solution is as simple as running npm install, but I think limiting how often these messages show up would be helpful.

@Arcanemagus
Copy link
Member

Personally I feel it should be left as is, if it is throwing an error then something is so wrong that it is unable to handle the situation. The only way it would be trying (and apparently failing) to load a local eslint is if you have configuration for one in your project, which means you have specifically said "I want this specific eslint to behave in this manner."
The package is simply trying to follow your wishes 😛

Thoughts @AtomLinter/linter-eslint?

@dominicbarnes
Copy link
Author

That's true, but I think it can communicate that something is wrong with a single error notification, and repeatedly spamming the notifications is never a good UX.

@steelbrain
Copy link
Contributor

Definately agree to that, I would recommend we do this in the core linter, so the feature is available to all linters

@SpainTrain
Copy link
Member

I actually think this is a reasonable enhancement. There are legitimate circumstances where the constant popup of the same error is just noise.

@MatthiasWinkelmann
Copy link

This should be solved upstream in atom/notifications#88 as it's an issue with many packages (i. e. linter-rubocop spams you with errors after adding to the Gemfile before you've had a chance to run bundle install).

@ryaninvents
Copy link

I'd prefer it if the errors did not throw notifications at all, but rather showed in the status line (instead of ✔️ No issues it could show ❌ eslint not found or similar). To me, the only time the error notifications should be used is if my workflow is completely blocked anyways; e.g. the editor view has crashed, and it's really worth my while to drop everything and fix the problem right away.

Sometimes I just want to make a quick edit in a repo where I haven't installed anything, but the linter errors are so loud that I end up using TextEdit. An improperly-configured linter isn't important enough that it ought to block my view of the text with brightly-colored notifications.

@SpainTrain
Copy link
Member

Given that atom/notifications#88 has been open for 16 months and activity on that project has been very low, are we open to implementing @r24y's proposal? If so, I can work up a PR.

@Arcanemagus
Copy link
Member

How about we transform it into a fake Linter message instead, just like has been done for invalid point errors in #761?

@ryaninvents
Copy link

@Arcanemagus that sounds perfect! That's probably the easiest way to implement it, and would be consistent across all linter UI packages.

@Arcanemagus
Copy link
Member

Arcanemagus commented Dec 1, 2016

Well, not currently, but that's the eventual idea 😉. So far this is an experiment in linter-flake8 and soon here in linter-eslint.

@alberto
Copy link

alberto commented Dec 5, 2016

Personally I feel it should be left as is, if it is throwing an error then something is so wrong that it is unable to handle the situation. The only way it would be trying (and apparently failing) to load a local eslint is if you have configuration for one in your project, which means you have specifically said "I want this specific eslint to behave in this manner."

Actually it's a problem for people developing eslint on Atom, like myself. Since we have a eslint config file but we don't depend on eslint, it uses the interal package from linter-eslint. There is some incompatibility from time to time between the internal package and master (mostly rule schema changes), and it's super noisy, so I have to disable linter-eslint (for all of my projects!) until it gets updated.

I wonder how you deal with this @IanVS ? We also added eslint-plugin-node as a dev dependency, which causes this same issue, and this one won't get fixed by upgrading. :(

@IanVS
Copy link
Member

IanVS commented Dec 6, 2016

@alberto, I haven't done a ton of work on ESLint lately, but when I did/do, I npm link in eslint, and then check the Use global ESLint installation option in linter-eslint (also making sure to install eslint-plugin-node globally). It's not ideal, but it does seem to work for me.

@dominicbarnes
Copy link
Author

dominicbarnes commented Dec 6, 2016

Sometimes, I dive into a package in node_modules/ directly to diagnose a quick problem, if one of those modules uses a .eslintrc, it continuously spams because eslint is missing in that directory.

That is probably an edge-case, but on second though I wonder if linter-eslint should bother running at all in VCS ignored files. (if we don't address the original issue of notification spam)

@Arcanemagus
Copy link
Member

@dominicbarnes Linter itself controls whether VCS ignored files are linted, and it defaults to not, maybe you re-enabled them at some point?

@alberto
Copy link

alberto commented Dec 6, 2016

Thanks @IanVS that probably helps, although it still worries me we cause this to contributors. It would be great if something could be done. Anyway, this is a great plugin, it made me decide to switch to Atom. Thanks ❤️

@skylize
Copy link
Contributor

skylize commented Sep 11, 2017

@dominicbarnes Here is something that vaguely resembles a workaround, and should at least make this slightly less annoying for you:

Whenever error messages are displayed, the escape key is mapped to closing all errors. So rather than grabbing the mouse and dragging it over to close each message, you can just press esc periodically as you proceed through your folders.

@Arcanemagus Arcanemagus self-assigned this Sep 17, 2017
Arcanemagus added a commit that referenced this issue Sep 17, 2017
Translate all errors coming from ESLint into a lint message instead of
throwing them out to be caught by Linter's generic error catching
mechanism. This allows us to preent them in a much cleaner manner and
gives the user immediate feedback on what is going wrong.

Fixes #387.
Arcanemagus added a commit that referenced this issue Sep 17, 2017
Translate all errors coming from ESLint into a lint message instead of
throwing them out to be caught by Linter's generic error catching
mechanism. This allows us to present them in a much cleaner manner and
gives the user immediate feedback on what is going wrong.

Fixes #387.
Arcanemagus added a commit that referenced this issue Sep 17, 2017
Translate all errors coming from ESLint into a lint message instead of
throwing them out to be caught by Linter's generic error catching
mechanism. This allows us to present them in a much cleaner manner and
gives the user immediate feedback on what is going wrong.

Fixes #387.
@skylize
Copy link
Contributor

skylize commented Sep 18, 2017

I think you guys are gonna be really happy with this #1015 merge. Thanks @Arcanemagus

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants