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

Feature: add rslint-ignore <rules> until <line> #23

Closed
RDambrosio016 opened this issue Oct 3, 2020 · 8 comments
Closed

Feature: add rslint-ignore <rules> until <line> #23

RDambrosio016 opened this issue Oct 3, 2020 · 8 comments
Assignees
Labels
C-enhancement Category: enhancement E-medium Fixing/implementing this issue is moderately hard and you may need some prior experience E-mentor T-Runner This issue primary relates to the lint runner, rslint_core

Comments

@RDambrosio016
Copy link
Collaborator

RDambrosio016 commented Oct 3, 2020

Currently rslint's ignore command is either scoped to the entire file or its scoped to the statement or declaration its on top of. This does not seem to be the norm apparently, i would like to keep scoping as it is and instead add a more explicit modifier to the ignore command as such:

ignoring an entire statement:

// rslint-ignore no-empty
if (foo) {}
else {} // no error

ignoring a block of code until another line:

// rslint-ignore no-empty until 5
{}
{}
{}
{}
{} // error

Perhaps we could also add rslint-ignore <rules> until eof? i really prefer an explicit command such as this over weird scoping you cannot make sense of, which is the case with eslint (in my opinion)

Mentoring instructions

To add this you will need to modify the directive parser which is found here, ignore commands are parsed here. What i would personally do is add two more variants to the commands enum called IgnoreRulesUntil(Vec<Box<dyn CstRule>>, Range<usize>) and IgnoreUntil(Range<usize>) where the ranges are start and end byte indices. You will need to cache the byte indices of line starts in the directive parser which is very easy, you can simply do it once when the directive parser is created, you can get the source code text through root_node.text().to_string(). To parse the actual directive you can simply peek the lexer to see if the next token's text is until and then further peek to see if its a number or eof. Then to enforce the directives you can simply add something to the place where rules are run, run through all of the directives, if any directive applies to the rule (ignoreUntil applies to all rules, IgnoreRulesUntil only applies if the rule is contained) then go through all of the diagnostics returned, if the first primary label of the diagnostic's range start is included in the directive's range then simply remove the diagnostic (Vec::retain can do this easily).

You will also need to update the docs here to document this.

@RDambrosio016 RDambrosio016 added T-Runner This issue primary relates to the lint runner, rslint_core E-mentor E-medium Fixing/implementing this issue is moderately hard and you may need some prior experience C-enhancement Category: enhancement labels Oct 3, 2020
@cristianossd
Copy link

It seems interesting, @RDambrosio016! I want to work on it

@RDambrosio016
Copy link
Collaborator Author

Go ahead! i heard from somebody that the directive parser's lexer is giving incorrect ranges sometimes, you may have to fix that too 👀

@cristianossd
Copy link

@RDambrosio016, I think some references on your mentoring instructions should be updated. Nowadays, the parser path is this one and where rules are run is that. Is this right?

@RDambrosio016
Copy link
Collaborator Author

yeah sorry, we moved everything to /crates and i forgot to update everything 😄

@Stupremee
Copy link
Member

Yea the directive parser is broken somewhere. You will have to fix it to use it. With some debugging it should be easy to find

@cristianossd
Copy link

Is it not better to create another issue to fix the parser or the broken part is strictly related to ignore rule feature?

@Stupremee
Copy link
Member

The current code for ignore and stuff works fine, so you can fix everything that you need in your PR

@RDambrosio016
Copy link
Collaborator Author

closing as we have decided this is not currently worth it over a disable then enable directive as line number can change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: enhancement E-medium Fixing/implementing this issue is moderately hard and you may need some prior experience E-mentor T-Runner This issue primary relates to the lint runner, rslint_core
Projects
None yet
Development

No branches or pull requests

3 participants