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

Ignore messages (false positives) #647

Closed
kankaristo opened this issue Jun 27, 2015 · 25 comments
Closed

Ignore messages (false positives) #647

kankaristo opened this issue Jun 27, 2015 · 25 comments

Comments

@kankaristo
Copy link

It would be nice to be able to ignore certain messages, e.g. false positives. Linters are most useful when you can keep the amount of errors and warnings at zero (ignoring false positives that can't be avoided).

For example, linter-less always gives this error in styles.less:

Error: 'ui-variables.less' wasn't found

That is a false positive, since Atom will actually find the file although the Less linter can't. It would be nice to be able to ignore this message (either the exact message, or using a regular expression).

(I found #187, but since that's closed, I thought it would be best to open a new issue.)

@steelbrain
Copy link
Owner

The last time I checked there are some linters that allow you to ignore certain error type, we can't really do it in the core linter. :)

@kankaristo
Copy link
Author

That's a shame. :(

And while a linter might allow ignoring e.g. all import/include messages, this would allow customizing this to fit a user's specific situation (which is unlikely to be added to a linter, even if the user made a pull request).

Edit: Which is also why I think it would be a valuable addition to the core linter, because it would be a user-specific setting, not linter-specific.

@steelbrain
Copy link
Owner

I know that you really want that feature, I agree that it sucks to see that less error in the panel all the time, so what approach would you suggest us?

@kankaristo
Copy link
Author

Could there be a global regex for messages to ignore? If a linter reports an error/warning that matches the regex, it would be removed from the list of messages.

So in the above example with ui-variables.less, the regex could simply be (ui\-variables\.less). Any linter message that matches that, would be removed.

Depending on the regex, this could be a massive performance hit, but as long as it's optional (or if the regex is simple), that wouldn't really become a problem.

This and #633 are really the only features that I wish were in Linter, otherwise it's pretty much perfect. :)

@steelbrain
Copy link
Owner

Have you seen how PhpStorm or WebStorm suppresses errors?
You put something like

// @noInspection name-of-inspection

and it ignores that inspection for the line above, now of course that wouldn't be the case for linter 'cause we know nothing about the provider's inspections and we just show the messages they give us.
But still we could have something like

// @linter-ignore

and the line below would be ignored, how does that sound to you?

@AsaAyers
Copy link
Contributor

That doesn't sound good to me. We don't know how comments work in whatever file you're working on and we shouldn't maintain mapping of comment types

# @linter-ignore
; @linter-ignore
// @linter-ignore
<!-- @linter-ignore -->
#_( @linter-ignore )
;; @linter-ignore

@kankaristo
Copy link
Author

A global regex would still be useful (as a separate feature), because (still keeping with the same example) the ui-variables.less error happens for every single Atom package with a style sheet, not just styles.less. This way you wouldn't need to mark every line with a false positive that can't be avoided (easily or at all).

Linter doesn't need any info about the message, just hide the message if it matches a regex?

@steelbrain
Copy link
Owner

@AsaAyers @k2b6s9j @lierdakil ^
thoughts?

@AsaAyers
Copy link
Contributor

No, this is outside the scope of the core. This issue should be opened with whatever linter produces the messages. eslint, jshint, and coffeelint all provide a native mechanism to comment and disable linting a section of code. So I'm not saying go open this issue in linter-less, it should be opened in less

@steelbrain
Copy link
Owner

I don't intend to use it personally, so all fine to me 😃

@kankaristo
Copy link
Author

Okay. :(

I still wish there was some generic way to ignore false positives, even though ideally any false positive should be fixed, not ignored. Sometimes real world limitations apply...

Thanks for your time! :)

@lierdakil
Copy link
Contributor

I think I have a vague idea of how to make this work, though. Since linter is attached to editor, it's possible to leverage underlying grammar to detect comments. Not necessarily arguing this should be implemented, but I guess it could be a nice feature. Feature creep might be an issue though, so I say put it in the "long drawer", and revisit this when more pressing issues (like disappointing lack of specs) are fixed.

@AsaAyers
Copy link
Contributor

when more pressing issues (like disappointing lack of specs) are fixed.

I look forward to reviewing your pull requests :)

@keplersj
Copy link
Contributor

I think this is outside of the core linter's responsibilities. The core is
responsible for giving providers an environment to function in and show
issues reliably. The provider I responsible for the issues themselves. If a
provider is providing messages that can be ignored it is up to the provider
not to provide them.

On Sat, Jun 27, 2015, 9:45 AM Steel Brain [email protected] wrote:

Reopened #647 #647.


Reply to this email directly or view it on GitHub
#647 (comment).

@AsaAyers
Copy link
Contributor

I'm not sure why this was reopened when it's clearly outside the scope of the core linter.

Yes, you can leverage the grammar, that's just a fancy way of saying what I did above. Linter needs to understand every possible grammar to know which typeof comment to look for.

@steelbrain
Copy link
Owner

@k2b6s9j I've marked it as a low priority for now, for some time in the future when we find this okay to implement.

@lierdakil
Copy link
Contributor

From a different viewpoint, universal syntax for ignoring messages could be nice for those of us who write in several languages, all with different linters (sometimes several).

Less cognitive overhead that way.

@lierdakil
Copy link
Contributor

@AsaAyers, linter doesnt need to know anything besides that a given snippet is marked as comment by grammar parser.

@lierdakil
Copy link
Contributor

@AsaAyers, that's literally 5 lines of code.

@steelbrain
Copy link
Owner

@AsaAyers Leaving this opened and keeping it a low priority wasn't gonna harm anyone, was it?
Also We're trying to make atom an IDE, and this feature is something that pushes us forward in that direction, also instead of doing it in every single provider, it would be compact to do it just in the core.

@kankaristo
Copy link
Author

@AsaAyers, please reconsider. In a perfect world, this would be implemented independently in each provider, or as a config option in a linter. But a generic way to ignore false positives would be really useful for custom/obscure build systems and project-specific situations, where this would be "out of scope" even for the providers/linters.

@AsaAyers
Copy link
Contributor

The provider isn't even normally the place to implement this. The linter (not us) should implement it. It's such a common thing that I've never used a linter that didn't already have the ability to ignore a section of code.

In fact linter-less seems to have exactly what you're asking for

linter-less is unusual in the sense that it seems to implement it's own linter. Ideally there would be a linter that isn't tied to Atom. Once you could call from the command line or any other tool and linter-less would simply use that.

@kankaristo
Copy link
Author

I have that feature turned on. It disables errors about undefined variables, not missing files for @import. Even if linter-less had a feature to ignore all import statements, I think that would be a poor solution to handle a single false positive.

The feature in this issue would also be a good "fallback" for a missing feature in a linter (like with linter-less in this case). I only used that as an example that is common for all Atom users.

@kankaristo
Copy link
Author

In my original description, I also mentioned #187, which is essentially the same as this (except I suggested the regex solution). That issue was also closed, but not fixed (as far as I know).

Edit: Actually, @nesmyslny also suggested the regex solution in that issue, I didn't notice that before.

@iam4x
Copy link
Contributor

iam4x commented Jun 28, 2015

I agree with others, not on the scope of this package. Should be linter specific, with eslint you can do:

/* eslint-disable */
console.log('this line is not linted');
/* eslint-enable */
var foo = 'bar';

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

No branches or pull requests

6 participants