-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[dev] Replace sass-lint with stylelint #86177
Conversation
sass-lint is unmaintained, so this replaces it with an actively maintained linter. The changeset here includes two entry points 1) node scripts/stylelint **/*.scss for general linting 2) via node scripts/register_precommit_hook on staged filed Lint rules and approach to migrating is TBD. I'm opening this now to showcase the code changes separately from a large lint changeset, and will circle back before merging depending on our approach.
This reverts commit 8311cd0.
Pinging @elastic/kibana-operations (Team:Operations) |
@@ -12,12 +12,12 @@ object Lint : BuildType({ | |||
|
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: also update the task description to replace sasslint with stylelint.
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.
pushed with b4d9ab8
@@ -18,4 +18,4 @@ | |||
*/ | |||
|
|||
require('../src/setup_node_env'); | |||
require('../src/dev/run_sasslint'); | |||
require('../src/dev/run_stylelint'); |
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.
Running this script directly outputs the stylelint help.
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.
Pushed d3cedd7. It uses the stylelint CLI parser to check for input globs, and if there aren't any we default to *.s(c|a)ss.
Co-authored-by: Tyler Smalley <[email protected]>
@elasticmachine merge upstream |
@snide I brought in most of the error rules from our sass-lint configurations. There were some missing analogues, e.g. border:none. Eyes from design would be helpful, we're targeting 7.12, so ~1 month if that's possible. I can push any review changes, or commits directly to this branch are fine. |
@jbudz I'll check this out later today. Thanks for doing the setup! |
Just an update here. This system works perefectly fine and I don't expect any changes on the engineering side. I'm adjusting some rules to be more strict and fixing sass as they fail. I should have a commit for this later today with a ✔️ . |
@jbudz PR for you. I added a couple dozen more rules and kicked the tires. Works great, and love that it can autofix. There's some more I'd like to add to this, but it can wait for a later PR when I have more time. IssuesAt least in VIM, I noticed that the prettier autosave settings were conflicting with the style-lint settings. I'd like to have @cchaos or someone check to see what happens in VSCode. I see there are some other extensions we can run for prettier similar to what we're doing with ESLint, but after messing with them for a bit, I wasn't smart enough to get them running. I want make sure this isn't just something my VIM extensions are doing wrong with prettier first, but that may cause problems for this PR if we're seeing it in other editors. |
clean up stylelint rules
@snide I was able to reproduce the prettier/stylelint conflict. The most straightforward option is to disable prettier on .scss files (pushed with a951ded) and rely on stylelint for style and errors. We haven't had any enforcement on prettier formatting to date, so it's hit or miss if our files are formatted properly. I looked at plugins - https://github.com/hugomrdias/prettier-stylelint looks like what we would want but it's not an active project. The third option is to align our style changes with prettier's default. I'm not seeing a way to do the reverse, e.g. prettier/prettier#2705. This would involve migrating towards https://github.com/stylelint/stylelint-config-standard/ and a closer look at the prettier defaults. Any thoughts? |
@jbudz I think that's a good option for now. People can use extensions in their editors for the style-lint warnings. Mostly wanted to make sure we didn't run into frustrations with people not being able to save files that style-lint liked. |
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.
This looks OK from design. We'll add to the rules over time in separate PRs. Thanks for setting this up.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Tyler Smalley <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dave Snider <[email protected]>
Co-authored-by: Tyler Smalley <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dave Snider <[email protected]> Co-authored-by: Tyler Smalley <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dave Snider <[email protected]>
7.x: 1dc3276 |
sass-lint is unmaintained, so this replaces it with
an actively maintained linter. The changeset here includes two entry
points
node scripts/stylelint
node scripts/register_precommit_hook
on staged filesCloses #79323