Skip to content
This repository has been archived by the owner on Jan 25, 2024. It is now read-only.

feat: format via alejandra when feature is set #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: format via alejandra when feature is set #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ log = "0.4.17"
lsp-server = "0.6.0"
lsp-types = { version = "0.93.0", features = ["proposed"] }
regex = "1.5.6"
alejandra = { version = "1.5.0", optional = true }
rnix = "0.10.2"
serde = "1.0.138"
serde_json = "1.0.82"
nixpkgs-fmt-rnix = "1.2.0"
nixpkgs-fmt-rnix = { version = "1.2.0", optional = true }

[dev-dependencies]
stoppable_thread = "0.2.1"
Expand All @@ -32,7 +33,7 @@ maplit = "1"
[features]

# Set this to ["verbose"] when debugging
default = []
default = [ "nixpkgs-fmt-rnix" ]

# Enable showing internal errors via editor UI, such as via hover popups.
# This can be helpful for debugging, but we don't want the evaluator to
Expand Down
42 changes: 33 additions & 9 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,16 +209,40 @@ impl App {
let document_links = self.document_links(&params).unwrap_or_default();
self.reply(Response::new_ok(id, document_links));
} else if let Some((id, params)) = cast::<Formatting>(&mut req) {
let changes = if let Some((ast, code, _)) = self.files.get(&params.text_document.uri) {
let fmt = nixpkgs_fmt::reformat_node(&ast.node());
vec![TextEdit {
range: utils::range(&code, TextRange::up_to(ast.node().text().len())),
new_text: fmt.text().to_string(),
}]
if let Some((ast, code, _)) = self.files.get(&params.text_document.uri) {
let node = ast.node();
let range = utils::range(&code, TextRange::up_to(node.text().len()));
if cfg!(feature = "alejandra") {
#[cfg(feature = "alejandra")]
match alejandra::format::in_memory(
params.text_document.uri.to_string(),
code.to_string(),
) {
(alejandra::format::Status::Changed(true), new_text) => {
self.reply(Response::new_ok(id, vec![TextEdit { range, new_text }]))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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:

  1. would you be open to having that feature implemented in the first place?
  2. how difficult do you think it would be to implement?
  3. do you have the capacity and willing to do this and if not, would you be open to a PR?

Copy link

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

}
(alejandra::format::Status::Changed(false), _) => {
self.reply(Response::new_ok(id, ()))
}
(alejandra::format::Status::Error(e), _) => {
self.reply(Response::new_err(id, ErrorCode::InternalError as i32, e))
}
}
} else if cfg!(feature = "nixpkgs-fmt-rnix") {
#[cfg(feature = "nixpkgs-fmt-rnix")]
self.reply(Response::new_ok(
id,
vec![TextEdit {
range,
new_text: nixpkgs_fmt::reformat_node(&node).text().to_string(),
}],
));
} else {
self.reply(Response::new_ok(id, ()))
}
} else {
Vec::new()
};
self.reply(Response::new_ok(id, changes));
self.reply(Response::new_ok(id, ()))
}
} else if let Some((id, params)) = cast::<HoverRequest>(&mut req) {
if let Some((range, markdown)) = self.hover(params) {
self.reply(Response::new_ok(
Expand Down