feat: format via alejandra when feature is set #89

rvolosatovs · 2022-06-23T17:58:04Z

Refs #80
Refs #81

Summary & Motivation

It's nice to give users freedom of choosing the formatter to use. I think that the correct approach would be to parse this from flake.nix formatter and/or expose this via LSP config.
For now, just added a simple solution, which is fully backwards-compatible and does not introduce additional dependencies.

When --features alejandra is specified, https://github.com/kamadorueda/alejandra is used as the formatter.
https://github.com/nix-community/nixpkgs-fmt is still the default, but can be disabled at compile-time via --no-default-features

Until merged this can be consumed via NixOS/nixpkgs@5ddf7f7 in nixpkgs

Ma27 · 2022-06-23T18:53:08Z

Why did you close this?

rvolosatovs · 2022-06-23T18:53:42Z

Why did you close this?

I tried it and it didn't work, pushing a fix!

rvolosatovs · 2022-06-23T18:58:40Z

alejandra crate was just published kamadorueda/alejandra#306, updated the PR

rvolosatovs · 2022-06-23T19:09:24Z

~~Flipped the order, now --features alejandra would make alejandra the preferred backend without having to --no-default-features as well~~ moved to OP

Ma27

Nice work!

Added a few minor remarks, also a test would be nice :)

Ma27 · 2022-06-27T09:58:37Z

Cargo.toml

@@ -24,15 +24,16 @@ rnix = "0.10.2"
 rowan = "0.12.6"
 serde = "1.0.104"
 serde_json = "1.0.44"
-nixpkgs-fmt-rnix = "1.2.0"
+nixpkgs-fmt-rnix = { version = "1.2.0", optional = true }
+alejandra = { version = "1.5.0", optional = true }


Have you checked how much it would increase the closure size if we just added both by default and make it configurable via e.g. the environment which formatter to use. I'm asking for two reasons:

I think it's realistic that it depends on a project which formatter is used (i.e. nixpkgs uses de-facto nixpkgs-fmt and a hypothetical other project may use alejandra).

I don't want users to recompile rnix-lsp just because they prefer alejandra (and I also don't want to expose multiple attributes for rnix-lsp in nixpkgs depending on the feature-set).

Also cc @kamadorueda for opinions.

As I laid out in the description, I think that and ideal solution would infer the formatter to use from the flake.nix file and maybe even directly rely on nix fmt to do that. In that case neither of these two dependencies would be necessary and nix would be the only dependency.
Until then, this would need to be configurable by the user via LSP config, such that it could be overriden on a per-project basis and for that we'd need to compile these in.

So until (and if) nix is integrated here directly, I think the project should absolutely include both by default. Of course, if more formatters emerge (https://xkcd.com/927/) that could become an issue. For just two I'd say it should be fine.

I did not implement handling this via LSP config to minimize the diff, but I am happy to do so if you prefer, although it does seem that the PR would be quite a bit bigger, since I'd have to implement LSP config handling from scratch, there's no existing functionality for that in the project yet, correct?

I'm not familiar with the LSP, but I can throw here that there is a third approach: using an environment variable pointing to the formatter binary, and just feeding the code via stdin and getting the output via stdout. That way the closure would decrease substantially as we would be reusing the binaries already present in the user system (instead of compiling and linking them here), we would not need to implement a configuration handler for this LSP, and we would unify the interface and allow for more formatters to be chosen in the future (including nix fmt)

Just an idea, you both are more familiar with this project than me

Of course, if more formatters emerge (https://xkcd.com/927/) that could become an issue

Well, the primary reason why I even consider this is because Alejandra seems to be used by quite a bunch of people and is IIRC even a candidate for the nixpkgs formatting RFC, so integrating it is justifiable IMHO.

That way the closure would decrease substantially as we would be reusing the binaries already present in the user system

Admittedly I haven't checked the actual impact on closure size of both alejandra and nixpkgs-fmt, but an LSP server is something that's only running on developer machines anyways, so I wouldn't consider a slight (i.e. non-optimal, but not "bad") increase of closure-size much of an issue.

I'm not familiar with the LSP, but I can throw here that there is a third approach: using an environment variable pointing to the formatter binary, and just feeding the code via stdin and getting the output via stdout.

As described in #87 partial reformats are desirable and something I'd like to see here as well. If we get somewhat reasonable APIs from formatters for that, I'd prefer that solution over having to parse stdout from a formatter. I could imagine that we'd also run into funny quoting or encoding issues when reformatting files with weird stuff in it and parse the (structured) output of the (incremental) edits from stdout.

Ma27 · 2022-06-27T10:00:31Z

src/main.rs

+ code.to_string(),
+ ) {
+ (alejandra::format::Status::Changed(true), new_text) => {
+ self.reply(Response::new_ok(id, vec![TextEdit { range, new_text }]))


We're implementing "partial" edits in #87 for nixpkgs-fmt, i.e. only the actual diffs, but not the reformatted file is returned.
AFAIU this also returns the entire file.

If we add a new formatter we should do it properly from the beginning, i.e. only return diffs. Otherwise it might be painful in the editor if e.g. all-packages.nix is attempted to be reformatted.

I believe that would require changes in alejandra to support this in the first place.
@kamadorueda https://github.com/kamadorueda/alejandra/blob/c685879cda91934f29f42574b8eb367496b09df1/src/alejandra/src/format.rs#L13-L40 is the only functionality I found for formatting text in-memory, is there something else I am not aware of?
If such functionality does not exist:

would you be open to having that feature implemented in the first place?

how difficult do you think it would be to implement?

do you have the capacity and willing to do this and if not, would you be open to a PR?

@rvolosatovs alejandra::format::in_memory is all we have

I understand why sending small TextEdits instead of a TextEdit with the whole file can be a good thing. The most simple approach I can think of is:

Use alejandra::format::in_memory to get a string with the formatted text

Use a diffing library to see which lines are equal in the input text and in the formatted text

Emit TextEdits for the ranges that are different

I'm not sure if that is good enough for this use case, I'm not familiar with LSP

would you be open to having that feature implemented in the first place?

That feature is more related to this repository than to Alejandra, so it would be better to have it here and let Alejandra just focus on being good at formatting

How much of a change would that be to Alejandra actually? The change in nixpkgs-fmt seemed rather trivial: nix-community/nixpkgs-fmt#296.

@Ma27 Would it be possible to land this even without partial reformatting? I'm working on projects that use alejandra, and my workflow ends up: edit, :w, :!nix run nixpkgs\#alejandra -- %, test, edit, start again. Even without partial reformatting this change will make my workflow much better. Could it be done separately later?

Not a maintainer of this anymore, so I don't have a strong opinion. Please discuss that with the current maintainers :)

Applying patch from nix-community/rnix-lsp#89 directly on top of current rnix-lsp and providing UseAlejandra command to switch to in when Vim is already running.

Signed-off-by: Roman Volosatovs <[email protected]>

rvolosatovs mentioned this pull request Jun 23, 2022

Publish alejandra_engine crate kamadorueda/alejandra#306

Closed

rvolosatovs closed this Jun 23, 2022

rvolosatovs reopened this Jun 23, 2022

rvolosatovs force-pushed the feat/alejandra branch 2 times, most recently from cb71399 to f99332e Compare June 23, 2022 18:57

rvolosatovs marked this pull request as ready for review June 23, 2022 18:57

rvolosatovs force-pushed the feat/alejandra branch from f99332e to 9189b50 Compare June 23, 2022 19:08

Ma27 reviewed Jun 27, 2022

View reviewed changes

feat: format via alejandra when feature is set

936ae39

Signed-off-by: Roman Volosatovs <[email protected]>

rvolosatovs force-pushed the feat/alejandra branch from 9189b50 to 936ae39 Compare February 2, 2023 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: format via alejandra when feature is set #89

feat: format via alejandra when feature is set #89

rvolosatovs commented Jun 23, 2022 •

edited

Ma27 commented Jun 23, 2022

rvolosatovs commented Jun 23, 2022

rvolosatovs commented Jun 23, 2022

rvolosatovs commented Jun 23, 2022 •

edited

Ma27 left a comment

Ma27 Jun 27, 2022

rvolosatovs Jul 1, 2022

kamadorueda Jul 3, 2022

Ma27 Jul 11, 2022

Ma27 Jun 27, 2022

rvolosatovs Jul 1, 2022

kamadorueda Jul 3, 2022

Ma27 Jul 11, 2022

YorikSar Dec 6, 2022

Ma27 Dec 6, 2022

feat: format via alejandra when feature is set #89

Are you sure you want to change the base?

feat: format via alejandra when feature is set #89

Conversation

rvolosatovs commented Jun 23, 2022 • edited

Summary & Motivation

Ma27 commented Jun 23, 2022

rvolosatovs commented Jun 23, 2022

rvolosatovs commented Jun 23, 2022

rvolosatovs commented Jun 23, 2022 • edited

Ma27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvolosatovs commented Jun 23, 2022 •

edited

rvolosatovs commented Jun 23, 2022 •

edited