KEMBAR78
Migrate `convert_match_to_let_else` assist to use `SyntaxEditor` by Hmikihiro · Pull Request #20218 · rust-lang/rust-analyzer · GitHub
Skip to content

Migrate convert_match_to_let_else assist to use SyntaxEditor #20218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 10, 2025
Merged

Migrate convert_match_to_let_else assist to use SyntaxEditor #20218

merged 1 commit into from
Jul 10, 2025

Conversation

@Hmikihiro
Copy link
Contributor

part of #15710 and #18285

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2025
@Hmikihiro
Migrate convert_match_to_let_else assist to use SyntaxEditor
be609a5 
Signed-off-by: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com>
@Hmikihiro Hmikihiro force-pushed the migrate_convert_match_to_let_else branch from f7d60e9 to be609a5 Compare July 10, 2025 02:22
ShoyuVanilla
ShoyuVanilla approved these changes Jul 10, 2025
View reviewed changes
Copy link
Member

@ShoyuVanilla ShoyuVanilla left a comment

Choose a reason for hiding this comment

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

Thanks!

@ShoyuVanilla ShoyuVanilla added this pull request to the merge queue Jul 10, 2025
Merged via the queue into rust-lang:master with commit df1c3e0 Jul 10, 2025
15 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2025
@Hmikihiro Hmikihiro deleted the migrate_convert_match_to_let_else branch July 10, 2025 05:10
@ShoyuVanilla
Copy link
Member

Oh, I missed little details while reviewing this:

rust-analyzer/crates/ide-assists/src/handlers/convert_match_to_let_else.rs

Lines 61 to 68 in cce300b

|builder| {
let extracting_arm_pat =
rename_variable(&extracting_arm_pat, &extracted_variable_positions, pat);
builder.replace(
let_stmt.syntax().text_range(),
format!("let {extracting_arm_pat} = {initializer_expr} else {diverging_arm_expr};"),
)
},

We still have a SourceChangeBuilder::replace call remaining here and I think this should be migrated as well, and instead of format!, SyntaxFactory methods for building asts.
And Ideally, I think passing &mut SyntaxEditor as a parameter to rename_variable is more desirable if possible.

@Hmikihiro Could you handle this as a followup? Only if you have enough time and are willing to take it on 😅

@Hmikihiro Hmikihiro mentioned this pull request Jul 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShoyuVanilla ShoyuVanilla ShoyuVanilla approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Hmikihiro @ShoyuVanilla @rustbot