-
Notifications
You must be signed in to change notification settings - Fork 280
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
Stylelint linter added in pre-hook #590
Conversation
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.
Overall looks great, @Ruffeng. Just a few minor tweaks and this will be good to merge.
README.md
Outdated
@@ -546,6 +546,7 @@ issue](https://github.com/brigade/overcommit/issues/238) for more details. | |||
* [SlimLint](lib/overcommit/hook/pre_commit/slim_lint.rb) | |||
* [Sqlint](lib/overcommit/hook/pre_commit/sqlint.rb) | |||
* [Standard](lib/overcommit/hook/pre_commit/standard.rb) | |||
* [StyleLint](lib/overcommit/hook/pre_commit/style_lint.rb) |
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.
Nit: If the actual project name is capitalized "Stylelint", then we should use that same capitalization, and have the file be named stylelint.rb
appropriately.
config/default.yml
Outdated
enabled: true | ||
description: 'Check styles with Stylelint' | ||
require_executable: 'stylelint' | ||
flags: ['-f compact'] |
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.
These need to be split into separate arguments ['-f', 'compact']
, unless the stylelint
CLI accepts -f compact
as a single argument, but that would be surprising.
config/default.yml
Outdated
@@ -711,6 +711,17 @@ PreCommit: | |||
install_command: 'npm install -g standard' | |||
include: '**/*.js' | |||
|
|||
StyleLint: | |||
enabled: true |
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.
We do not enable hooks by default unless they are truly applicable to almost any project. Set this to false
.
@sds Thanks for your comments. I hope now will be ok 👍 |
This PR is in response with #514, which @brianbancroft started a PR but never had a chance to finish it.
The main problem with this linter was the kind of output that Stylelint was giving. Overcommit wasn't ready to handle multiline errors for the same file. So I asked if it could be possible to do a compact format for that stylelint#3415.
Thanks to @ntwb, that was possible (PR 3488). Therefore, I was able to implement the new linter at overcommit.
To run properly the linter in the CLI:
stylelint "**/*.css" -f compact
. I would really appreciate if someone can verify I added correctly the way to run it indefault.yml
I would like to say that this PR is made based on @briancroft 's PR. He deserves part of the contribution. As well @ntwb to make it possible from the Style lint side
Feel free to code review and verify everything is ok 👍