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.

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

Implement Client Rename Provider Settings #4950

Closed
wants to merge 20 commits into from

Conversation

Razmo99
Copy link

@Razmo99 Razmo99 commented Mar 25, 2024

PR Summary

This pull request implements the VSCode RenameProvider class along with two methods within it provideRenameEdits and prepareRename the goal being to complete #261

This will allow for the client to make requests to the server side of the extension when pressing F2 to perform refactoring on variables and functions.

This client side code has been modified from the C# VSCode extension and modified to work with the Powershell extension's server side component.

There will be a sister pull request on the PowershellEditorServices repository to implement the server side.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • [NA] PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

@Razmo99
Copy link
Author

Razmo99 commented Mar 25, 2024

@microsoft-github-policy-service agree

@Razmo99 Razmo99 changed the title WIP: Implementing Client side Rename provider to allow variable/function rename Implementing Client side Rename provider to allow variable/function rename Mar 25, 2024
@Razmo99 Razmo99 marked this pull request as ready for review March 25, 2024 04:06
@Razmo99 Razmo99 requested a review from a team March 25, 2024 04:06
@SydneyhSmith
Copy link
Collaborator

Hi @Razmo99 super appreciate the detailed PR! Just want to give you a heads up that the maintainers are all pretty heads down on commitments at the moment so it may be a couple of weeks before we get a chance to review the PR

@silverqx
Copy link

silverqx commented May 31, 2024

I would like to ask, what is the state of this PR, does it work actually? I fastly scrolled through the proposed PR and also PowerShellEditorServices#2152, looks like @Razmo99 did a great job about registerRenameProvider and worked ~1 year on this.

The code quality looks good to me and contains a lot of tests, which is a good thing.

Would it be possible to review it, git checkout it, test it, and release it in the next version? @andyleejordan

I lack renaming in PS extension so much, would greatly boost dev. productivity in many cases. I know there always is Search & Replace, but properly implemented rename refactorings are gold in some cases 🙂 To just hit ctrl+shift+r or f2 and rename the symbol under the cursor, to avoid classic search and replace.

I know that the PR is huge, but also the functionality is complex.

@andyleejordan
Copy link
Member

I would really like to get this feature in, yes. However, we're currently paused on feature work for a bit while some security work takes priority. Said another way: please expect to see it within the year, but probably not in the next month.

@silverqx
Copy link

silverqx commented Jun 1, 2024

Ok, thx for stating it out, would be nice to make it a little quicker but whatever, at least check-in it out and test it to see if it works and in which state the PR is would be a good idea. I think that having a new feature like this is more interesting for developers than having npm packages at the latest versions every month or every release 😁

@JustinGrote
Copy link
Collaborator

For what it's worth I'm currently reviewing the PSES part of this to hopefully help reduce the friction when the opportunity arises for a more thorough review.

@andyleejordan
Copy link
Member

Justin's work will be very helpful and should speed up the process!

src/features/RenameSymbol.ts Outdated Show resolved Hide resolved
src/features/RenameSymbol.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
@JustinGrote
Copy link
Collaborator

Here is a build of the latest PSES rename changes. Lots of progress to go but it's here to test out:
powershell-2024.6.0-renameAlpha.vsix.zip

You'll need to rename the .vsix.zip to just .vsix and then install using VSIX locally with vscode.

@JustinGrote
Copy link
Collaborator

JustinGrote commented Jun 5, 2024

Caught here too:

$NeededTools = @{
    OpenSsl       = 'openssl for macOS'
    PowerShellGet = 'PowerShellGet latest'
    InvokeBuild   = 'InvokeBuild latest'
}

function getMissingTools () {
    $missingTools = @()

    if (needsOpenSsl) {
        $missingTools += $NeededTools.OpenSsl
    }
    if (needsPowerShellGet) {
        $missingTools += $NeededTools.PowerShellGet
    }
    if (needsInvokeBuild) {
        $missingTools += $NeededTools.InvokeBuild
    }

    return $missingTools
}

If you try to rename $NeededTools inside the function, it only renames the external reference but not the internal ones. Vice-versa does not work, if we support one we should support the other.

Maybe this should be a configurable option? Or maybe we require SCRIPT or GLOBAL scope references in order for rename of variables referenced outside functions to work? Probably good to put a warning here that if you try to rename an inner referenced variable but the initial reference is not found, should fail with a nice error message explaining that.

Should maybe look at the AST logic of PSUseDeclaredVarsMoreThanAssignments as a reference (or even call it directly) for finding those references.

@andyleejordan
Copy link
Member

Hi! We discussed this in triage and our main condition on accepting it is that it's an opt-in feature controlled by a setting that ideally has a good explanation of when and how it can fail (due to PowerShell scoping). As long as it's opt-in most of our concerns don't apply.

@JustinGrote
Copy link
Collaborator

JustinGrote commented Jun 6, 2024

Hi! We discussed this in triage and our main condition on accepting it is that it's an opt-in feature controlled by a setting that ideally has a good explanation of when and how it can fail (due to PowerShell scoping). As long as it's opt-in most of our concerns don't apply.

Right, I was thinking more specifically for this handling, but I plan to assist it in such a way that it will operate as such:

First time you attempt F2 rename, a popup will appear with a link to Markdown docs we will have in the docs section explaining about PowerShell being a dynamic language, and the limitations and at-your-own-risk aspects. You will then have to click an "Accept", "Accept for Workspace", etc. option that will drop a setting in the appropriate location to suppress the dialog and allow rename to continue.

@andyleejordan andyleejordan force-pushed the main branch 2 times, most recently from 486e86e to ef038c8 Compare June 6, 2024 23:17
package.json Outdated Show resolved Hide resolved
@JustinGrote
Copy link
Collaborator

@Razmo99 this is great work, FYI, I will be PTO June 14-28 in Europe so I may be limited in further reviews until then but I will probably show this off at PSConfEU and give you full credit!

@Razmo99
Copy link
Author

Razmo99 commented Jul 15, 2024

@JustinGrotes, are you back in action from Europe? just wondering what are next steps are to move forward. Do you want me to implement the opt in mechanism on the client side?

@JustinGrote
Copy link
Collaborator

@Razmo99 thanks for following up and all your hard work. I've had some life circumstances come up that are going to unfortunately minimize my involvement as a volunteer at the moment, I hope I'll be able to get involved again soon, but I think in the meantime as mentioned by @andyleejordan the opt-in triggered on first F2 rename, controlled by a setting is a good next step as well as preparing a markdown document that details the expectations (no multi-file for now, etc.) and the edge cases. He would ultimately be performing/approving the merge so his guidance would be paramount to the merge.

@JustinGrote
Copy link
Collaborator

Force pushed a rebase to deal with the silly not-really-merge-conflicts.

@JustinGrote
Copy link
Collaborator

@Razmo99 since I've had some time to dive into this, is there a particular reason you made a custom PowerShell/prepareRename rather than using the standard TextDocument/prepareRename that the LSP uses? Since we don't get the full documentUri, that helps explain why unsaved/untitled files don't work right.

@Razmo99
Copy link
Author

Razmo99 commented Sep 25, 2024

I didn't know there was a standard prepearRename provider 😅 I made it the way I did as I wanted a separate endpoint to send rename requests to the language server and handle them differently rather them complicating the main rename symbol endpoint

@ALIENQuake
Copy link

Great feature, one suggestion, the initial variable name should not contain $ in order to not require repeating this char when providing new name.

@JustinGrote
Copy link
Collaborator

Great feature, one suggestion, the initial variable name should not contain $ in order to not require repeating this char when providing new name.

There is logic in the latest commits where it doesn't matter if you have the $ or not, it'll get fixed automatically, and it has checking to make sure it's a valid variable name (e.g. no spaces unless theres a {} around the name, etc.)

@JustinGrote JustinGrote changed the title Implementing Client side Rename provider to allow variable/function rename Implement Client Rename Provider Settings Oct 2, 2024
@JustinGrote
Copy link
Collaborator

I'm closing this PR in favor of #5056 since the scope has drastically changed. @Razmo99 you should get co-author credit on the PR, if not we'll make sure it ends up in the release notes for sure.

@JustinGrote JustinGrote closed this Oct 2, 2024
@Razmo99
Copy link
Author

Razmo99 commented Oct 2, 2024

I'm closing this PR in favor of #5056 since the scope has drastically changed. @Razmo99 you should get co-author credit on the PR, if not we'll make sure it ends up in the release notes for sure.

@JustinGrote no problem at all, honestly I'm just happy to see this finally get implemented as a feature of this plugin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants