Change default formatter for any language

[PiergiorgioZagaria]

I have never worked on a big project in rust, so I don't know if the Result/Option handling is ok.

This PR aims to allow the user to choose which formatter to use instead of always relying on lsp. Discussion about this in issue #2362.

Currently the only problem is that write_all_impl saves only the viewed buffers, this is because the formatter needs to reload or apply the transaction to the document. How do we get around this?

[PiergiorgioZagaria]

Another bug I found using zig fmt --stdin, the editor hangs because the formatter doesn't understand that stdin has wrote everything. I tried pushing a 0 to the collected vector and flushing stdin, but neither seems to work.

The expected usage of zig fmt --stdin is: $ cat sometext.zig | zig fmt --stdin

[archseer]

You probably need to close stdin when you're done.

[PiergiorgioZagaria]

You probably need to close stdin when you're done.

Yeah that was it, thanks

[PiergiorgioZagaria]

Thanks for the style guides btw.

[PiergiorgioZagaria]

I found another bug. Basically, when using Stdio as the formatter type, when saving, the document is formatted and saved, but the editor shows that the file is still unsaved

[PiergiorgioZagaria]

I don't know anything about async, ideas on how to make this work?

// let fut = async move {
//     helix_core::diff::compare_ropes(self.text(), &Rope::from_str(str))
// }
// return Ok(fut);

line 458 of document.rs.
(Btw how do I put the reference to the code in the comment?)

[sudormrfbin]

I don't know anything about async, ideas on how to make this work?

// let fut = async move {
//     helix_core::diff::compare_ropes(self.text(), &Rope::from_str(str))
// }
// return Ok(fut);

line 458 of document.rs.

Document::format() originally returned an Option<impl Future<...>> which used the impl Trait syntax in the return type of the function. This means that the function will return some type implementing Future, but it also enforces that we can only really return a single concrete type (see this SO post about using impl Trait syntax for returning multiple types).

Now, the actual problem is that different async blocks generate different types that implement Future and therefore we can't use impl Future in our return type. We can use a trait object to solve this, and a BoxFuture is simply a type alias for Pin<Box<dyn Future<Output = ...>>> provided by the futures_util crate (which is what I ended up using).

(Btw how do I put the reference to the code in the comment?)

https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-a-permanent-link-to-a-code-snippet

[PiergiorgioZagaria]

Thanks for all the help and the explanation.

I guess the only problem now is the fact that write_all only writes to viewed buffers instead of all buffers, because we need the view for reloading the document if we are using the file formatter.

Maybe we could remove the file formatter type entirely? It depends on whether there are other known formatters that only work like that.

[PiergiorgioZagaria]

Since I haven't found any formatter that doesn't support the stdio type, I suggest removing formatter type completely and use only stdio.
If there is a need for a file formatter type it should be implemented in a separate PR.

[samholmes]

This PR should include changes to documentation?

[PiergiorgioZagaria]

Probably we should mention it in the languages configuration section

[samholmes]

Can we access the current file name for the formatter config? This is needed for eslint:

formatter = { command = "eslint", args = ["--fix", "$HELIX_FILE"] }

Is there something like this?

[PiergiorgioZagaria]

Helix sends the content of the file through stdin to the formatter. I think eslint has a --stdin flag that should work

[kirawi]

Probably we should mention it in the languages configuration section

You could add it here: https://docs.helix-editor.com/guides/adding_languages.html

[samholmes]

The problem with eslint is that it wont return the file if it's able to fix. Instead, it returns a formatted response possibly containing the file output. For example, specifying the json format:

/usr/local/bin/eslintfmt

#!/bin/zsh

eslint --stdin --fix-dry-run --format=json | jq '.[0].output' <&0

This is close, except if the output in the JSON is not define because there was an error, then the file is emptied of contents. This script would need to parse the output and return the formatted output if there are no issues, otherwise return the stdin. I'm guessing this has already been written somewhere.

[archseer]

Let's tackle the write_all problem in a separate PR, the fix there should be to just pick any view_id that already has a selection for that document. I had to do that somewhere else in commands.rs if I remember correctly

[archseer]

Great work on this!

[PiergiorgioZagaria]

I think I forgot to mention it, but when we removed the file formatter in favour of just using stdio formatting, the problem with write_all was also solved. That is because with the file formatter we needed the view_id to reload the window, but now that we just use transactions the problem doesn't exist

[David-Else]

Brilliant! Could anyone provide an example for prettier to format markdown files?

[PiergiorgioZagaria]

I don't know the cli flags for prettier, assuming that the default behaviour is that it takes from stdin and prints to stdout, so for example you would call it like
cat file.md | prettier. Then you would just need to put

[[language]]
name = "markdown"
formatter = { command = "prettier" }

In your languages.toml

[David-Else]

Cheers! I tested it with cat file.md | It should be:

[[language]]
name = "markdown"
formatter = { command = "prettier --parser markdown" }

Do I need any escape charcters for the command? In Neovim it was --parser\ markdown. Also, if there is an error does it abort? In Neovim the buffer just became the error message.

[PiergiorgioZagaria]

Then it's
formatter = { command = "prettier", args = ["--parser", "markdown"] }

[kirawi]

Also, if there is an error does it abort? In Neovim the buffer just became the error message.

It'll return an error:

helix/helix-view/src/document.rs

Lines 443 to 451 in 0ee2061

	if !output.stderr.is_empty() {
	return Err(FormatterError::Stderr(
	String::from_utf8_lossy(&output.stderr).to_string(),
	));
	}
	if !output.status.success() {
	return Err(FormatterError::NonZeroExitStatus);
	}

[dam5h]

@PiergiorgioZagaria , thank you for this! It's great to be able to use black as a python formater but pyright for LSP. pyright doesn't support formatting.

One question is related to the auto-format option in the editor config, as the default is true, shouldn't this happen automatically on saving a file, or was that circumvented when we gave this formatter priority over lsp?

EDIT: Just realized there is more local auto-format setting in the languages config. Even though editor had true, python had false. So all is good, thanks again!

[dead10ck]

Stderr doesn't always mean there's an error. There are programs that use it to print diagnostic messages so that e.g. you can still pipe stdout to a file and see informational messages in the output. The only indication of error we can really use reliably is the exit code. What we should probably do is move this whole if expression inside the following one that checks the exit code. The enum variants can probably change the enum to get rid of Stderr and add an Option<String> to NonZeroExitStatus.

[PiergiorgioZagaria]

Ok, I'll open a new PR to fix it.
Btw the function shell_impl in helix-term/src/commands.rs, which is used when calling the shell with ! Or | doesn't implement these checks, should we add them there too?

[dead10ck]

I see, in that case, yeah, those need to be fixed too. The presence of output on stderr does not necessarily mean there was an error.

[kirawi]

You can use from_reader here instead. I think it is more efficient. Or str::from_utf8.

[PiergiorgioZagaria]

std::str::from_utf8 works, but str::from_utf8 doesn't. Btw for further fixes to this part you can comment on #3349

[theherk]

It's great to be able to use black as a python formater but pyright for LSP. pyright doesn't support formatting.

I case anybody stumbles in here searching for how to configure black, this is the configuration that does the trick.

[[language]]
name = "python"
auto-format = true
formatter = { command = "black", args = ["-", "-q"] }