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

Bug: rule name prefixed with some characters works in deno-lint-ignore #287

Closed
magurotuna opened this issue Aug 27, 2020 · 9 comments · Fixed by #494
Closed

Bug: rule name prefixed with some characters works in deno-lint-ignore #287

magurotuna opened this issue Aug 27, 2020 · 9 comments · Fixed by #494
Assignees

Comments

@magurotuna
Copy link
Member

// deno-lint-ignore xxxxxxxxxxxxban-types
let a: Object;

You can see that the rule name is wrong, prefixed with many x characters. We expect this directive not to have an effect.
However, it does works, just as we write a directive correctly like // deno-lint-ignore ban-types.

On the other hand, rule names suffixed with characters like ban-typesxxxxxxxxdo not work, which is the behavior we expect.

I read the code and then found the cause at:

deno_lint/src/linter.rs

Lines 103 to 108 in 14f1543

// `ends_with` allows to skip `@typescript-eslint` prefix - not ideal
// but works for now
if code.ends_with(&diagnostic.code) {
should_ignore = true;
*self.used_codes.get_mut(code).unwrap() = true;
}

The comment says that using ends_with is to skip @typescript-eslint prefix.
This means we want deno-lint to be able to handle rule names prefixed with @typescript-eslint, right?
If so, we could probably realize it by removing @typescript-eslint prefix (if exists) when parsing a comment in the function parse_ignore_comment. In this way, I guess the bug I mentioned above will be fixed.

@bartlomieju
Copy link
Member

@magurotuna that's correct. To be honest neither of these solutions is perfect and it should be probably handled by having "aliases" for rules, but I haven't yet figured how to do that.

It was changed in #269. Feel free to revert to previous solution.

@magurotuna
Copy link
Member Author

@bartlomieju hmm... it's kind of more complex problem than I thought. :-|
Could you tell me why you think my solution (removing prefix when parsing) is not perfect? It won't work in some cases?

@bartlomieju
Copy link
Member

@bartlomieju hmm... it's kind of more complex problem than I thought. :-|
Could you tell me why you think my solution (removing prefix when parsing) is not perfect? It won't work in some cases?

It's still hardcoded and won't work for rules with other plugins - but I guess we can not care ATM. So feel free to bring back previous behavior.

@magurotuna
Copy link
Member Author

Thank you for the explanation. Understood.
I think this bug is not so serious. So it's OK to pend until some great solution occurs to us.

@bartlomieju
Copy link
Member

@magurotuna I've been pondering on this issue for some time. I think we can solve it by using RuleContext instead of LintContext - that way each rule could register multiple "codes" it recognizes (eg. ban-types, @typescript-eslint/ban-types).

Ignoring of produced diagnostics would happen when processing each lint rule instead of doing it in bulk at the end.

That should help us get rid of the hacky code you pointed in the first comment.

@magurotuna
Copy link
Member Author

@bartlomieju
sounds great, that way we'll be able to solve this problem.

@bartlomieju bartlomieju self-assigned this Oct 6, 2020
@bartlomieju
Copy link
Member

After some more pondering I think we shouldn't support aliases after all and require the proper rule names from deno_lint.

@bartlomieju
Copy link
Member

@magurotuna what do you think?

@magurotuna
Copy link
Member Author

@bartlomieju

we shouldn't support aliases after all and require the proper rule names from deno_lint

that makes sense to me.

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