stevearc / conform.nvim

Lightweight yet powerful formatter plugin for Neovim
MIT License
2.55k stars 135 forks source link

feat!: expand options for LSP formatting #456

Closed stevearc closed 2 weeks ago

stevearc commented 3 weeks ago

This deprecates the lsp_fallback option (gracefully; it should continue to work) and replaces it with the lsp_format enum which can be several values. Three of the values produce behavior that was possible with lsp_fallback, and two of them are new behaviors. The new behaviors fix #316

lsp_format lsp_fallback description
nil or "never" nil or false Do not use LSP formatting
"fallback" true Use LSP formatting if there are no formatters configured for this filetype
"prefer" - Use LSP formatting when it is available, otherwise fall back to the formatters configured for this filetype
"last" "always" Run configured formatters, then run LSP formatting
"first" - Run LSP formatting, then run configured formatters
folke commented 2 weeks ago

Hi!

I'm getting multiple reports from users that the lsp fallback doesn't work anymore. Haven't looked into it myself.

dpetka2001 commented 2 weeks ago

I don't know if this is an intended change or not, but the previous fallback behavior didn't check if there was a formatters_by_ft defined for the current buffer, so it just used lsp formatting as a last resort.

Now, with this line introduced in the latest update, it seems that it also checks if there's no filetype formatter defined in formatters_by_ft, which is clearly different from before. This leads to confusion to users who were accustomed to previous behavior, that used LSP formatting if no other formatting was used before. Again, I don't know if this is intended behavior or not in the latest update.

folke commented 2 weeks ago

@dpetka2001 nice find!

The LSP should definitely be used as a fallback. @stevearc I assume this change was not intentional?

dpetka2001 commented 2 weeks ago

Also, this function M.will_fallback_lsp does not have any references after the last update. So, it's not being used. Previously, it was used here. So, I believe more changes would have to be introduced other than the line that checks for the filetype formatters. Unfortunately, I don't understand the codebase that well to really understand how this should be solved and can't submit a PR. Sorry about that.

dpetka2001 commented 2 weeks ago

Did my best and submitted a PR that fixes this behavior if it's not intentional. Please feel free to disregard it if you think a better approach is possible, as I'm not that familiar with the codebase myself.

stevearc commented 2 weeks ago

This behavior change is not intentional, it was supposed to be a transparent API change with no effect on behavior. I'll take a look at the PRs and make sure a fix gets in soon

stevearc commented 2 weeks ago

Should be fixed by bde3bee1773c96212b6c49f009e05174f932c23a