Skip to content
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

Prompt for name when using extract refactoring assists #17579

Open
joshka opened this issue Jul 11, 2024 · 7 comments
Open

Prompt for name when using extract refactoring assists #17579

joshka opened this issue Jul 11, 2024 · 7 comments
Labels
A-assists A-ide general IDE features C-feature Category: feature request C-tracking-issue Category: tracking issue

Comments

@joshka
Copy link
Contributor

joshka commented Jul 11, 2024 •
edited
Loading

In typescript, extracting a variable / function / etc. allows you to enter a name for the just created extraction. It would be nice to do the same in RA for functions, modules, variables, etc.

image image

Playing with the extract_variable:

if let Some(cap) = ctx.config.snippet_cap {
    if let Some(ast::Pat::IdentPat(ident_pat)) = let_stmt.pat() {
        if let Some(name) = ident_pat.name() {
-            edit.add_tabstop_before(cap, name);
             edit.add_placeholder_snippet_group(
                 cap,
                 vec![name.syntax().clone(), name_expr.syntax().clone()],
            )
        }
    }
}

SourceChangeBuilder::add_placeholder_snippet_token doesn't seem quite right for this. Instead of a single edit location which applies to both places when the edit is confirmed, this method makes two edit locations that are edited simultaneously.

image

I'm not quite sure what the right terminology to use to find the functionality here, so seeking some guidance on what to do to make this work.

Edit: this UX is provided by the rename symbol function, but I'm not sure how that would fit in to the extract refactoring.
@joshka joshka added the C-feature Category: feature request label Jul 11, 2024
@DropDemBits
Copy link
Contributor

It seems that the Typescript language server is sending a command to the client (in this case, VSCode) to trigger a rename dialog, as the dialog that shows up for renaming a symbol is shown by the client, which then sends the new name as part of a textDocument/rename request.

rust-analyzer does have custom commands (see

rust-analyzer/editors/code/src/main.ts

Line 113 in 5577e4e

function createCommands(): Record<string, CommandFactory> {
), but you'll have to look into the VSCode Typescript extension to see how they do it.

@DropDemBits DropDemBits added the A-ide general IDE features label Jul 11, 2024
@joshka
Copy link
Contributor Author

joshka commented Jul 12, 2024

Thanks for the guidance on where to look.

Looking at tsserver, it seems that every refactoring method returns (possibly empty) information on the file and location to use in presenting the rename command after the refactor finishes.

In RA, currently there's a trigger_signature_help bool, which controls whether a command is returned from the code action to the vscode client (and I assume is then run by the client). I think this can possibly be implemented by replacing the trigger_signature_help bool with an enum that specifies what command will run after the refactoring. I'll experiment a bit more soon.

joshka pushed a commit to joshka/rust-analyzer that referenced this issue Jul 13, 2024
@bors @joshka
Trigger VSCode to rename after extract variable assist is applied
8efe8a8 
When the user applies the "Extract Variable" assist, the cursor is
positioned at the newly inserted variable. This commit adds a command
to the assist that triggers the rename action in VSCode. This way, the
user can quickly rename the variable after applying the assist.

Fixes part of: rust-lang#17579
@joshka joshka mentioned this issue Jul 13, 2024
Merged
@joshka
Copy link
Contributor Author

joshka commented Jul 13, 2024

Added #17587 which triggers editor.action.rename after performing variable extraction. I'd also add function and module extraction. Are there any other assists I might have missed that would also need this behavior?

bors added a commit that referenced this issue Jul 15, 2024
@bors
Auto merge of #17587 - joshka:jm/edit-name-after-refactor, r=Veykril
e961b1a 
Trigger VSCode to rename after extract variable assist is applied

When the user applies the "Extract Variable" assist, the cursor is
positioned at the newly inserted variable. This commit adds a command
to the assist that triggers the rename action in VSCode. This way, the
user can quickly rename the variable after applying the assist.

Fixes part of: #17579

https://github.com/user-attachments/assets/4cf38740-ab22-4b94-b0f1-eddd51c26c29

I haven't yet looked at the module or function extraction assists yet.
@Skgland Skgland mentioned this issue Jul 15, 2024
Open
@DropDemBits
Copy link
Contributor

From looking at ide-assists/src/handlers, extract_type_alias would be another good candidate. The other good candidates would be the generate assists (e.g. generate_trait_from_impl, generate_function).

Note that not all of the generate assists use the correct snippet APIs (see #17332), and to do so would require migrating them to use both mutable AST and the appropriate add_placeholder_snippet methods.

@joshka
Copy link
Contributor Author

joshka commented Jul 15, 2024 •
edited
Loading

Thanks. I took a brief look at the folder and also saw

  • add_lifetime_to_type
  • extract_module
  • extract_struct_from_enum_variant
  • extract_type_alias
  • extract_variable
  • generate_constant
  • generate_function
  • generate_trait_from_impl
  • promote_local_to_const (maybe)

Note that not all of the generate assists use the correct snippet APIs (see #17332), and to do so would require migrating them to use both mutable AST and the appropriate add_placeholder_snippet methods.

I wondered about that - good to know that these don't implement the right methods. With 1.5K issues, it seems like it would be difficult to build up the context of what's a problem and what's intended easily.

@DropDemBits
Copy link
Contributor

  • extract_struct_from_enum_variant

I'd also classify this one as a maybe as you'd typically want the extracted struct to have the same name as the variant name.

With 1.5K issues, it seems like it would be difficult to build up the context of what's a problem and what's intended easily.

Guess you're in luck for this domain, since I was the one who moved the assists to using the add_placeholder_snippet method 😁

@joshka
Copy link
Contributor Author

joshka commented Jul 15, 2024

I'd also classify this one as a maybe as you'd typically want the extracted struct to have the same name as the variant name.

makes sense.

Guess you're in luck for this domain, since I was the one who moved the assists to using the add_placeholder_snippet method 😁

Neat - it's good to be lucky :)

@Veykril Veykril added the C-tracking-issue Category: tracking issue label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-assists A-ide general IDE features C-feature Category: feature request C-tracking-issue Category: tracking issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@joshka @Veykril @DropDemBits