feat: make format timeout configurable.

[ayamir]

No description provided.

[charliie-dev]

lmao, i'm just doing a conform.nvim migration lol

[misumisumi]

When using a formatter from none-ls, a timeout appears if the formatter does not support range format and the format takes a long time.
lsp-format-modifications doesn't respect none-ls timeout?

When I set format_modifications_only to false, the timeout defined in settings.lua worked.

It may be necessary to limit the formatters to those in joechrisellis/lsp-format-modifications.nvim/issues/1 (whitelist format).

[misumisumi]

I worked when setting to vim.lsp.buf.format({ timeout_ms = opt.timeout_ms }) however timeout is shown as before.
Do we add this conf in format function ?

[ayamir]

lmao, i'm just doing a conform.nvim migration lol

what's the progress?

[ayamir]

When using a formatter from none-ls, a timeout appears if the formatter does not support range format and the format takes a long time. lsp-format-modifications doesn't respect none-ls timeout?

When I set format_modifications_only to false, the timeout defined in settings.lua worked.

It may be necessary to limit the formatters to those in joechrisellis/lsp-format-modifications.nvim/issues/1 (whitelist format).

Yes, we should do more things for null-ls.

[charliie-dev]

what's the progress?

u can merge this pr first if it's ready.

[Jint-lzxy]

Yes, we should do more things for null-ls.

I think we could create a wrapper around the function (or in terms of the config, format_callback) that handles the real formatting inside lsp-format-modifications.nvim. This way, we can unify those "formatting providers" (e.g., LSP, none-ls) even without lsp-format-modifications.

[Jint-lzxy]

lmao, i'm just doing a conform.nvim migration lol

@CharlesChiuGit Did u find anything in conform.nvim that's (practically) better than our "native" formatting approach? I've been thinking about making the switch, but I'm still on the fence lol

[charliie-dev]

@CharlesChiuGit Did u find anything in conform.nvim that's (practically) better than our "native" formatting approach? I've been thinking about making the switch, but I'm still on the fence lol

@Jint-lzxy
TBH, not much really.

1. conform.nvim uses "filetype"-based config v.s. none-ls.nvim uses linter-based config. Can't tell which is better, it's just a matter of taste.
2. https://github.com/zapling/mason-conform.nvim vs https://github.com/jay-babu/mason-null-ls.nvim
3. i think the best part of conform is that it supports formatting injections, for example, formatting code blocks in markdown files.( i'm not sure if none-ls can do it tho.)

I'm also working on refactoring formatting.lua since i think current setup is too complicate lol.
Here's my temporary config for conform.lua and conform_util.lua.

I'll finish it asap. plz let me know if u have any ideas or we could just stick to none-ls.

[charliie-dev]

after trying to migrate to conform.nvim, i think it's not a obvious advantage to do the migration.

[Jint-lzxy]

@CharlesChiuGit

( i'm not sure if none-ls can do it tho.)

afaiu, yes and no.

The "yes" part is bc we only need a little code to extend the existing API and make it work. The "no" part is that injection formatting is, as of now, still not part of the formal standard, so things can break accidentally if the standard changes and a "standard-conforming" server is being used.

But honestly, I don't find that very useful lmao most of the time I'm just using an autocmd to extract these embedded codes, format them individually, and then place them back 😂

I'm also working on refactoring formatting.lua since i think current setup is too complicate lol.

lol that's cooooool! Hope we get a sneak peek very soon 😄

plz let me know if u have any ideas or we could just stick to none-ls.

TL;DR: I don't think delegating separate document capabilities (e.g., formatting, linting) to different plugins is really a good idea. imho it's better to implement the "client-side stuff" on our end (except for the builtins) and leave everything else to a comprehensive management framework that implements the protocol itself. i.e.,:

        ┌──────────────────┐                                        
        │Other Capabilities│                                        
        │                  │                                        
  ┌─────►  vim.diagnostic  │                                        
  │     │     vim.lsp      │                                        
  │     │     extmark      │                                        
  │     │       ...        │           ┌──────────────────┐         
  │     └──────────────────┘     ┌─────►       LSPs       ├───┐     
  │                              │     │ (nvim-lspconfig) │   │     
  │ Delegates                    │     └──────────────────┘   │     
  │                              │                            │     
┌─┴────────┐                     │                      ┌─────▼────┐
│  Client  │── ── ── ── ── ── ── │◄─────────────────────┤ Servers  │
│ (Neovim) │     JSON-RPC        │                      │(Multiple)│
└─┬────────┘                     │                      └─────▲────┘
  │                              │     ┌───────────┐          │     
  │                              │     │   exec    │          │     
  │                              └─────► (none-ls) ├──────────┘     
  │ Delegates                          └───────────┘                
  │                                                                 
  │                                                                 
  │     ┌─────────────────────┐                                     
  │     │Formatting Capability│                                     
  └────►│                     │                                     
        │  > formatting.lua   │                                     
        └─────────────────────┘                                     

Long answer: lol if you ask me, I'd say I actually prefer sticking to none-ls. bc if I recall correctly, the bare bones of the LSP, apart from those fancy features like semantic tokens, are to support (a) formatting, (b) linting, and (c) AST parsing (i.e., achieving a certain degree of "understanding" of the code as a whole). And none-ls just does that. nvim-lspconfig is for servers that actually follow the specification, whereas none-ls makes the others work "as if" they implement the standard protocol.

Personally, I feel like plugins such as conform.nvim and nvim-lint indeed do their jobs pretty well, but their use cases can all be covered by none-ls. Plus, they usually don't have that "comprehensive" amount of builtins as compared to none-ls, which makes me question the reason for including two instead of just one that would do both jobs, and even better (in my workflow at least).

Also, the client-side stuff we really need to handle is actually just textDocument/*formatting (which isn't applied automatically by Neovim itself). Other features, such as visualizing linting results, are already implemented in the vim.diagnostic module and work pretty well. All of these reasons combined are why I chose to stick with none-ls lol

[charliie-dev]

@Jint-lzxy

lol that's cooooool! Hope we get a sneak peek very soon 😄

lol, i'm not going to refactor it now since my draft is based on conform.nvim and since now we're going to use conform.nvim so no need to refactor now(

edit: lol, i'm not going to refactor it now since my draft is based on conform.nvim and since now we're not going to use conform.nvim so no need to refactor now(

[Jint-lzxy]

lol, i'm not going to refactor it now since my draft is based on conform.nvim and since now we're going to use conform.nvim so no need to refactor now(

Sorry, but did u mean "we're NOT going to use conform.nvim"? A bit confused when reading this lol

[charliie-dev]

@Jint-lzxy

Sorry, but did u mean "we're NOT going to use conform.nvim"? A bit confused when reading this lol

yeah, after trying to config conform.nvim, i don't see obvious advantages to do so. And the config is not even cleaner.

[Jint-lzxy]

yeah, after trying to config conform.nvim, i don't see obvious advantages to do so. And the config is not even cleaner.

@CharlesChiuGit lol I actually meant to point out a possible typo in ur comment (looked like u missed a "not"), but anyway, thanks for the extra comments 😄

lol, i'm not going to refactor it now since my draft is based on `conform.nvim` and
since now we're going to use `conform.nvim` so no need to refactor now(
               ^
               ^ missing "not" here?

[charliie-dev]

yeah, lol it's a typo

[charliie-dev]

i think we can merge this pr now

[Jint-lzxy]

i think we can merge this pr now

@CharlesChiuGit iiuc we still need to implement #1275 (comment) (or sth equivalent) right? (Original issue: #1275 (comment))

[ayamir]

@misumisumi Could you please check it again?

[misumisumi]

settings["format_modifications_only"] = false seems to work perfectly.
timeout is effective.
In case settings["format_modifications_only"] = true, the following problem occurs:

• stylua formats with incorrect indentation
• unsupported formatters will not format at all

You might want to add an experimental note if you don't care about modification_only.

[ayamir]

stylua formats with incorrect indentation

It can be repro for stylua but not for other formatters. It's the reason why my commit doesn't pass CI.

[Jint-lzxy]

It can be repro for stylua but not for other formatters.

I think I mentioned elsewhere that range formatters only capture the lines that have actually been changed, without necessarily being aware of the context, which can occasionally lead to errors.

[ayamir]

I think I mentioned elsewhere that range formatters only capture the lines that have actually been changed, without necessarily being aware of the context, which can occasionally lead to errors.

So it's a upstream bug? Perhaps use treesitter to capture the context of changes lines and commit it to the formatter is a solution. For example, stylua supports the input of --range-start and --range-end: https://github.com/JohnnyMorganz/StyLua?tab=readme-ov-file#formatting-ranges

[Jint-lzxy]

So it's a upstream bug? Perhaps use treesitter to capture the context of changes lines and commit it to the formatter is a solution.

Hmm I think it's not. afaiu this is how the specification defines this capability. Additionally, the link u shared mentions the following:

Only whole statements lying within the range will be formatted. If part of a statement falls outside the range, the statement will be ignored.

[Jint-lzxy]

unsupported formatters will not format at all

@misumisumi Do u have any examples for this case? We do resort to reformatting the entire buffer if range formatting fails for any reason.

if
	format_modifications_only
	and require("lsp-format-modifications").format_modifications(client, bufnr).success
then
	if format_notify then
		vim.notify(
			string.format("[LSP] Format changed lines successfully with %s!", client.name),
			vim.log.levels.INFO,
			{ title = "LSP Range Format Success" }
		)
	end
	return
end

-- Fall back to format the whole buffer (even if partial formatting failed)
local params = vim.lsp.util.make_formatting_params(opts.formatting_options)
local result, err = client.request_sync("textDocument/formatting", params, timeout_ms, bufnr)
...

[misumisumi]

Do u have any examples for this case? We do resort to reformatting the entire buffer if range formatting fails for any reason.

textlint have problem.

[ayamir]

@misumisumi Seems it has been formatted when range format failed?

[misumisumi]

Yes.
This is the result when range format is enabled.
It works when range format is not enabled.

[misumisumi]

I checked NullLsLog, and it seems that the formatter is not even called in the first place.
The same is true for shfmt, which does not support range formatting.
This occurs with files managed by git.
Also, files before they are managed by git can be formatted.
However, textlint fails with a timeout, perhaps ignoring timeout_ms.
(textlint inserts very heavy processing)

null-ls returns require("lsp-format-modifications").format_modifications(client, bufnr).success as true regardless of the result.

[ayamir]

The same is true for shfmt, which does not support range formatting.

In my test case, the format of the whole file is executed when range format failed, which is expected.

IMO this PR is ready to merge b/c it has been realize its original goal, i.e., make format timeout configurable.

[Jint-lzxy]

LGTM