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

feat(stylelint): add StylelintFixAll command #2089

Closed
wants to merge 2 commits into from

Conversation

bennypowers
Copy link

@bennypowers bennypowers commented Aug 25, 2022

end

local request
if opts.sync then
Copy link
Member

@justinmk justinmk Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of thing is why in #1937 I'm very hesitant to add these special-case commands to each lspconfig. It's unpleasant for users to have a different interface or every random command. Here we have a function with an "async" flag. Looks like it's always true currently. This is a lot of code, and only for one command. I don't see how we can maintain 100s of configs with this much code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the record I don't like either of these commands and I think both are workarounds.

That being said, I just want to fix my stylelint errors on save. I have no idea why autoFixOnSave and autoFixOnFormat are not respected, but this config and an aucmd will will solve the problem for users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennypowers

require('lspconfig')['stylelint_lsp'].setup({
    settings = {
      stylelintplus = {
        autoFixOnFormat = true,
      },
    },
})

plus lukas-reineke/lsp-format.nvim seems to be fixing stylelint errors for me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link, but this pr is for support in lspconfig without an additional plugin

I'm definitely going to check that out for my own config tho

@glepnir glepnir mentioned this pull request Aug 26, 2022
@bennypowers
Copy link
Author

I agree with @glepnir and @justinmk's comments about how bloated this repo is and that the goal should be to release language-specific plugins

That being said, I still think this should be merged, in order to support the simple user who just wants to format and fix their file on save, rather than telling them to wait for a big rewrite.

@glepnir
Copy link
Member

glepnir commented Aug 26, 2022

@bennypowers if this is a necessary improve it can be :) I am not familiar the stylelint need check and test it .if anyone can give some advice very thanks then.

@bennypowers
Copy link
Author

bennypowers commented Aug 26, 2022

set neovim up as described in this pr (with autocmds), then

git clone https://github.com/bennypowers/nvim-stylelint-repro
cd nvim-stylelint-repro
npm ci
nvim styles.css

write file

expect file contents to equal

body {
  background-color: red;
}

@glepnir
Copy link
Member

glepnir commented Aug 26, 2022

but as the justin said there has vim.lsp.handler and opts. sync so I need follow the justin his advice.

@justinmk
Copy link
Member

That being said, I still think this should be merged, in order to support the simple user who just wants to format and fix their file on save, rather than telling them to wait for a big rewrite.

That's always the justification provided for each of these new features. This is how scope-creep occurs.

Not seeing a strong justification here.

We need a very thin approach (2-3 lines of code, not 50+ as in this PR) to adding support for custom (off-spec) server features. Until that happens (perhaps never), we need to stop adding custom commands. #1937

@justinmk justinmk closed this Aug 30, 2022
@bennypowers bennypowers deleted the fix/stylelint/autofix branch August 30, 2022 12:09
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.

4 participants