-
-
Notifications
You must be signed in to change notification settings - Fork 73
Linter: Use --generate-todo to create a todo list for ignoring rules. #685
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
base: main
Are you sure you want to change the base?
Linter: Use --generate-todo to create a todo list for ignoring rules. #685
Conversation
f9b1910 to
2c29a62
Compare
|
|
||
| if (!this.linter) { | ||
| this.linter = new Linter(Herb) | ||
| this.linter = new Linter(Herb, undefined, this.linterTodo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing undefined to let the linter set the default rules is not very idiomatic. Maybe it is better to pass the default rules on each call 🤷🏻 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can refactor the Linter to take in an options object as the second argument. So it could be:
| this.linter = new Linter(Herb, undefined, this.linterTodo) | |
| this.linter = new Linter(Herb, { rules: customRules, linterTodo: this.linterTodo }) |
or just:
| this.linter = new Linter(Herb, undefined, this.linterTodo) | |
| this.linter = new Linter(Herb, { linterTodo: this.linterTodo }) |
|
@marcoroth happy to receive feedback on this. Currently a little WIP and can refactor a little more, but it works well, tried on buk-webapp monolith and is working good (4288 erb files). Maybe i can follow up this PR with the following:
|
| } | ||
| } | ||
|
|
||
| export class LinterTodo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is not the best. Maybe TodoList? I belive that just Todo could fit too, but Todo could mean many more things.
2c29a62 to
ea420de
Compare
| --no-timing hide timing information | ||
| --no-wrap-lines disable line wrapping | ||
| --truncate-lines enable line truncation (mutually exclusive with line wrapping) | ||
| --generate-todo generate a .herb-todo.yml file with current diagnostics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be --regenerate-todo as rubocop, or --generate-todo as standardrb. I think regenerate makes clear that you are destroying the old one. But generate-todo is a little less verbose and simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can have both so there is no surprise.
--generate-todo can generate a file if none is present yet. If one is present it could back out and say something like: .herb-todo.yml already exists, run "--regenerate-todo" to overwrite it.
--regenerate-todo would just always overwrite the current one, no matter if .herb-todo.yml exists or not.
9d06eeb to
dd6f773
Compare
Using this parameter creates a .herb-todo.yml file that is used to ignore rules massively, this is usefull for big projects adopting the linter, to start ignoring all current offenses without having to ommit all in the files manually.
dd6f773 to
30ecd96
Compare
|
When you run Maybe we can mention that we generated a On subsequent runs, I think it would also be great to mention the amount of "suppressed offenses", so people are aware that the linter is only passing because of the todos in the |
|
Another thing (which might not be as common in day-to-day use-cases), but I run the Running a) Put the excludes:
html-no-space-in-tag:
../../../../rubyevents/app/views/topics/show.html.erb:
warning: 0
error: 1
../../../../rubyevents/app/views/templates/new.html.erb:
warning: 0
error: 3
# ...Maybe we can also mention the absolute path of which Thank you so much for working on this @domingo2000! 🙏🏼 |
Solves #667
Use
--generate-todoto create a todo list that will ignore all the current offenses based on an offense count. Very similar of how rubocop does.The implementation writes to a
.herb-todo.ymlfile, that stores a map of all the files and rules with the counts of offenses and warnings. When the linter runs, reads from that file and ignore all ocurrences declarated asexclude.The file is not dependant on a Herb Config file, because is autogenerated and can be very big, it should be consider as "private".
The
--generate-todoname comes from standardrb recommendation on todo list.Usage:
TODO