-
-
Notifications
You must be signed in to change notification settings - Fork 41
feat: improved reformat edits #87
feat: improved reformat edits #87
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, mostly some questions regarding the implementation here and in the corresponding nixpkgs-fmt PR.
Also, do you think you'd be able to write a small integration test in src/tests.rs
? :)
@@ -323,6 +325,24 @@ pub fn selection_ranges(root: &SyntaxNode, content: &str, pos: Position) -> Opti | |||
root.map(|b| *b) | |||
} | |||
|
|||
pub fn atom_edit_to_tuple(atom_edit: AtomEdit) -> (ops::Range<usize>, String) { | |||
let r: std::ops::Range<usize> = atom_edit.delete.start().into()..atom_edit.delete.end().into(); | |||
(r, atom_edit.insert.to_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to document what this is doing and also to add a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done in 4cc254a.
@@ -323,6 +325,24 @@ pub fn selection_ranges(root: &SyntaxNode, content: &str, pos: Position) -> Opti | |||
root.map(|b| *b) | |||
} | |||
|
|||
pub fn atom_edit_to_tuple(atom_edit: AtomEdit) -> (ops::Range<usize>, String) { | |||
let r: std::ops::Range<usize> = atom_edit.delete.start().into()..atom_edit.delete.end().into(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the two dots really needed here or is that a Rust feature I'm not aware of? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ..
operator creates a Range
from two numbers. Since AtomEdit
's delete
property is of type TextRange
, which belongs to the text-size
crate, I'm converting that range type to the builtin std::ops::Range
type to avoid adding dependencies to textedit-merge
.
new_text: fmt.text().to_string(), | ||
}] | ||
let (spacing_edits, indent_edits) = nixpkgs_fmt::reformat_edits(&ast.node()); | ||
let merged_edits = textedit_merge::merge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why this is needed, but I guess this can be answered with nix-community/nixpkgs-fmt#296 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, like I mentioned in nix-community/nixpkgs-fmt#296 (comment), the delete ranges in the spacing edits apply to the original document, and the indent edits apply to the document after the spacing edits have been applied. Since the language server protocol expects a single set of edits in response to a reformat request, I believe merging the two sequential sets of edits into a single set of edits is necessary.
Thanks! I've responded to the questions here and on nix-community/nixpkgs-fmt#296; let me know if there's anything that's still unclear. I'd be happy to add the two tests you mentioned, though I might not get to it until later in the week. |
I've added an integration test for reformatting in 5ffd970. The one thing I noticed while doing that is that editor indentation preferences are not respected. I don't think there's anything we can do about that though as long as we're using |
5ffd970
to
6beffc1
Compare
Ok, I've rebased things against the latest master and switched the remote for |
6beffc1
to
a284cf4
Compare
a284cf4
to
d79a372
Compare
@Ma27 I'd like to get this merged at some point; I've been using these changes for about two months and things seem good. Would you be able to create a new release of https://github.com/nix-community/nixpkgs-fmt on crates.io, or should I open an issue on that repository? |
I resigned as maintainer here, cc @aaronjanse |
I'm closing this because I've switched to using https://github.com/oxalica/nil, so I haven't been using or maintaining these changes for a while. Also, at one point while I was still using these changes, I did run into one situation where applying the formatting broke the code. So I think there must be a bug somewhere in either nixpkgs-fmt or in the textedit-merge package that I wrote for this merge request, but I don't have an example to reproduce the issue unfortunately... |
Summary & Motivation
The current method of reformatting documents consists of replacing the whole document with a single edit containing the reformatted version. This is sub-optimal for two main reasons:
This pull request updates the reformatting so that it sends multiple small edits, instead of one huge edit.
Further context
Since accomplishing this requires exposing the edits that
nixpkgs-fmt
uses internally, this depends on nixpkgs-fmt#296.This pull request also adds one additional dependency: textedit-merge, which I wrote in the proccess of creating this PR.
nixpkgs-fmt
reformats the document with two sets of edits (spacing edits, then indentation edits), however, from my understanding, the language server protocol requires that the response to a reformatting request be a single set of textedits. Since the ranges in the indentation edits used bynixpkgs-fmt
refer to the state of the document after the spacing edits have been applied, a somewhat complex algorithm is required to merge the two sets of edits into a single set of edits that can be applied in one step.One implementation detail that I would like a second opinion on are the data types:
nixpkgs_fmt::reformat_edits
functiontextedit_merge::merge
textedit_merge::merge
The current choices for these data types (
(Vec<AtomEdit>, Vec<AtomEdit>)
,&[(Range<usize>, AsRef<str>)]
, andVec<(Range<usize>, String)>
respectively) were chosen with the following goals in mind:nixpkgs_fmt::reformat_edits
(so that other users of this function can use it efficiently)textedit-merge
(so that other users, if there ever are any, aren't tied to additional dependencies)The main shortcoming of the current types is that the edits must be transformed twice, once from
Vec<AtomEdit>
toVec<(Range<usize>, AsRef<str>)>
for merging, then a second time fromVec<(Range<usize>, String)>
toVec<TextEdit>
so they can be returned in the response to the reformatting request. If there's a way to do things that still achieves both of those goals while avoiding the double transformation, please let me know!Finally, it would be dishonest of me if I didn't mention that I'm not 100% confident that
textedit-merge
is bug-free. I've tested reformatting onnixpkgs-fmt
's test cases and it works correctly on all of them now, but while I was working on it, I missed quite a few edge cases that those tests brought to light. If anyone can think of edge cases that might break edit merging, please give them a try! I'd much rather fix bugs withtextedit-merge
before merging this than find out about them later, as many of the bugs that I encountered during development resulted in panics since they involved indexing strings by invalid ranges.Closes #81.