Skip to content

fixers/stylelint: enhance stylelint fixer#2745

Merged
w0rp merged 6 commits into
dense-analysis:masterfrom
charlesbjohnson:charlesbjohnson/fixer/stylelint/use-stdin
Oct 28, 2019
Merged

fixers/stylelint: enhance stylelint fixer#2745
w0rp merged 6 commits into
dense-analysis:masterfrom
charlesbjohnson:charlesbjohnson/fixer/stylelint/use-stdin

Conversation

@charlesbjohnson
Copy link
Copy Markdown
Contributor

@charlesbjohnson charlesbjohnson commented Sep 1, 2019

👋

This upgrades the stylelint fixer. A few things have been done:

  1. The fixer now supports let g:ale_stylelint_options for custom flags
  2. The fixer now supports stdin by virtue of --stdin-filename (available in 6.3.0)
  3. The fixer now changes the working directory before running stylelint

(much of this is inspired from fixers/prettier)

I believe the combination of 2 & 3 may resolve a few issues since it allows any stylelint plugins to find project configuration files as expected (similar to #2622). This may fix:

@charlesbjohnson
Copy link
Copy Markdown
Contributor Author

charlesbjohnson commented Sep 13, 2019

Please hold on review, I'm running into a bug where the error output is actually replacing the buffer when using stdin.

Seems to affect other syntaxes (ie. SCSS):

echo '.foo- {' | npx stylelint --fix --stdin-filename foo.scss
foo.scss
 1:1  ✖  Unclosed block   CssSyntaxError
echo '.foo- {' | npx stylelint --fix --stdin-filename foo.css
.foo- {
}

@charlesbjohnson
Copy link
Copy Markdown
Contributor Author

charlesbjohnson commented Sep 13, 2019

According to stylelint/stylelint#3633, it seems to only happen when a non-CSS --syntax is used.

Without any stylelint configuration for SCSS:

echo '$color:red; .foo { &-bar { color:$color } }' | npx stylelint --fix --stdin-filename foo.css
$color:red; .foo { &-bar { color:$color } }

With stylelint configuration for SCSS (ie. stylelint-scss with scss/dollar-variable-colon-newline-after) and valid syntax:

echo '$color:red; .foo { &-bar { color:$color } }' | npx stylelint --fix --stdin-filename foo.css
$color:
red; .foo { &-bar { color:$color } }

With stylelint configuration for SCSS and invalid syntax (as demonstrated previously):

echo '.foo- {' | npx stylelint --fix --stdin-filename foo.css
.foo- {
}

As such, I've made a change to force a .css extension after the stdin filename. So far it seems to resolve the issue for me, but perhaps better to let it bake for some time 🍞 first.

@w0rp
Copy link
Copy Markdown
Member

w0rp commented Oct 7, 2019

Does this work acceptably well for other file formats?

@charlesbjohnson
Copy link
Copy Markdown
Contributor Author

I have not yet tested with other file formats since I've found that this workaround still isn't a good enough solution. I'm still occasionally running into issues where stylelint dumps a stacktrace to stdout and blows away my buffer.

Let me know how you want to proceed. My guess is that it's either a bug in stylelint or there's some flag that I'm unaware of. I have yet to take a look at how stylelint plugins for other editors are handling this.

@w0rp
Copy link
Copy Markdown
Member

w0rp commented Oct 7, 2019

If the --stdin-filename argument can cause some problems, maybe take that away for now and keep just the other changes which shouldn't cause any issues, and bring that back later when the issues have been resolved.

@charlesbjohnson
Copy link
Copy Markdown
Contributor Author

take that away for now and keep just the other changes which shouldn't cause any issues

Done. Two things worth noting:

  1. It seems that using a temporary file is still an issue for configuration detection. With stylelint-prettier, for example, prettier is unable to detect a project configuration file since it starts trying to find one from the file being formatted. This is essentially the same thing as feat(template-lint): Read from stdin #2622, and it will be resolved when we can use --stdin-filename without issue.

  2. I believe I misjudged the issues I linked in the PR description. The behavior in those issues is similar, though, and I think the fix would be similar to what we're doing here but just for a different tool (ie eslint vs stylelint). These changes probably won't fix them.

bring that back later when the issues have been resolved

Sounds good, stay tuned 📺

Thanks!

@w0rp w0rp merged commit af8c851 into dense-analysis:master Oct 28, 2019
AntoineGagne added a commit to AntoineGagne/ale that referenced this pull request Oct 31, 2019
* master: (39 commits)
  Fix the test issues with html-beautify
  Add support for html-beautify (dense-analysis#2788)
  fixers/stylelint: enhance `stylelint` fixer (dense-analysis#2745)
  Fix dense-analysis#2835 - Bump up the sign group version check for NeoVim
  Mention the disabled option for message severity
  Adding support for LSP `window/showMessage` method (dense-analysis#2652)
  Fix tsserver not returning details for items with empty source
  Allow code actions to work on callback based sources
  Add support for nimlsp (dense-analysis#2815)
  Add definition of c/clangd's language as C (dense-analysis#2791)
  Bump the ALE version
  Fix TCP server config example.
  Suboptimal fix to prevent variables from leaking out of new clangd test
  Hopefully fixed issue with Windows paths
  Added tests for clangd compile commands dectection
  Updated ale_linters/c/clangd.vim to match ale_linters/cpp/clangd.vim
  Fix dense-analysis#2800 - Ignore completion user data which is not a dictionary
  Fix dense-analysis#2821 - Fix the debride linter after merging older code
  Add the possiblity to add extra psalm options
  fix tflint handler for 0.11+ (dense-analysis#2775)
  ...
timlag1305 pushed a commit to timlag1305/ale that referenced this pull request Nov 5, 2019
* Refactor stylelint fixer test
* Support additional stylelint fixer options
* Support changing working directory for stylelint fixer
* Force css syntax for stylelint fixer
@charlesbjohnson charlesbjohnson deleted the charlesbjohnson/fixer/stylelint/use-stdin branch April 9, 2020 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants