Migrate merge_imports Assist to Use SyntaxFactory
#18484
Conversation
rustbot
added
the
S-waiting-on-review
|
Marking this as a draft for now since I’m running into an error. ---- handlers::merge_imports::tests::test_merge_with_synonymous_imports_1 stdout ----
thread 'handlers::merge_imports::tests::test_merge_with_synonymous_imports_1' panicked at crates/syntax/src/syntax_editor/edit_algo.rs:79:8:
some replace change ranges intersect: [Replace(Token([email protected] "\n\n"), Some(Token([email protected] "\n\n"))), Replace(Node([email protected]), None), Replace(Token([email protected] "\n"), None), Replace(Node([email protected]), Some(Node([email protected])))]
---- handlers::merge_imports::tests::test_merge_with_synonymous_imports_2 stdout ----
thread 'handlers::merge_imports::tests::test_merge_with_synonymous_imports_2' panicked at crates/syntax/src/syntax_editor/edit_algo.rs:79:8:
some replace change ranges intersect: [Replace(Token([email protected] "\n\n"), Some(Token([email protected] "\n\n"))), Replace(Node([email protected]), None), Replace(Token([email protected] "\n"), None), Replace(Node([email protected]), Some(Node([email protected])))]
---- tests::generated::doctest_merge_imports stdout ----
thread 'tests::generated::doctest_merge_imports' panicked at crates/syntax/src/syntax_editor/edit_algo.rs:79:8:
some replace change ranges intersect: [Replace(Node([email protected]), Some(Node([email protected]))), Replace(Token([email protected] "\n"), Some(Token([email protected] "\n"))), Replace(Node([email protected]), None), Replace(Token([email protected] "\n"), None)] |
|
I'll review this tomorrow |
| @@ -68,24 +68,31 @@ pub(crate) fn merge_imports(acc: &mut Assists, ctx: &AssistContext<'_>) -> Optio | |||
| (selection_range, edits?) | |||
| }; | |||
|
|
|||
| // FIXME: Is this the best to get a `SyntaxNode` object? We need one for `SourceChangeBuilder::make_editor`. | |||
There was a problem hiding this comment.
I think that we can get node for SyntaxEditor inside the above if-else blocks; like let (node, target, edits) = if ctx.has_empty_selection() { ..
| .collect(); | ||
| for edit in edits_mut { | ||
| let mut editor = builder.make_editor(&parent_node); | ||
| for edit in edits { |
There was a problem hiding this comment.
I think that to avoid the intersection assertion failure, we should use SyntaxEditor::replace_all for consecutive text ranges instead of individual insert or delete calls
There was a problem hiding this comment.
So the edits are generated in a vector by calling either use_item.try_merge_from() or use_tree.try_merge_from(). How can we combine all of these edits into a single replace_all() call?
|
☔ The latest upstream changes (presumably #18483) made this pull request unmergeable. Please resolve the merge conflicts. |
Signed-off-by: Tarek <[email protected]>
Signed-off-by: Tarek <[email protected]>
Signed-off-by: Tarek <[email protected]>
tareknaser
force-pushed
the
syntax_factory_merge_imports
branch
from
ca0b516 to
01eaf9a
Compare
Signed-off-by: Tarek <[email protected]>
Veykril
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #18657) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR is part of #15710 and #18285.
Changes Made
SyntaxEditor#18285 for migration.edits_mutvector fromcrates/ide-assists/src/handlers/merge_imports.rsSyntaxFactorymethodwhitespacewhitespaceas it mirrorsmake::tokens::whitespacebuttokens_whitespacemight be a more fitting name.edit_in_place::Removabletrait withedit_in_place::EditorRemovable(Relevant conversation here)SyntaxEditorinstance in theremovemethod to use instead ofted.ast::Useandast::UseTreeSyntaxEditor::deleteinstead ofted::removeandted::remove_all_iter. SinceSyntaxEditor::removedoesn’t exist, I assumed delete would perform the same function—though this might be causing issues.