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

Stylelint linter added in pre-hook #590

Merged
merged 2 commits into from
Jul 30, 2018
Merged

Stylelint linter added in pre-hook #590

merged 2 commits into from
Jul 30, 2018

Conversation

Ruffeng
Copy link
Contributor

@Ruffeng Ruffeng commented Jul 29, 2018

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 in default.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 👍

Copy link
Owner

@sds sds left a 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)
Copy link
Owner

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.

enabled: true
description: 'Check styles with Stylelint'
require_executable: 'stylelint'
flags: ['-f compact']
Copy link
Owner

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.

@@ -711,6 +711,17 @@ PreCommit:
install_command: 'npm install -g standard'
include: '**/*.js'

StyleLint:
enabled: true
Copy link
Owner

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.

@Ruffeng
Copy link
Contributor Author

Ruffeng commented Jul 30, 2018

@sds Thanks for your comments. I hope now will be ok 👍

@sds sds merged commit 22229c9 into sds:master Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants