-
Notifications
You must be signed in to change notification settings - Fork 510
F2 (rename) doesn't work for PowerShell variables #1440
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
Comments
I can confirm that this isn't working and it is disappointing. :( |
Ok, I've investigated this and we haven't ever supported it -- I was under the impression that we once did and it broke, but that's not the case. Marking this as up-for-grabs in case anybody else wants to try implementing it (we can help you through). |
@rjmholt I would like to give it a try if I can be pointed in the right direction |
Awesome! There are going to be a few pieces to the puzzle, but basically it will look like:
Keybindingvscode-powershell/package.json Lines 86 to 114 in 2607974
You'll need to add a configuration here and then a new feature with a request type, like this one: vscode-powershell/src/features/ShowHelp.ts Lines 1 to 49 in 0fb852c
Request HandlerOnce you've turned the keybinding into a request, you need to add a request handler on the back end in PSES. Those get added here: You also need to create request and response types for deserialisation like this one: https://github.com/PowerShell/PowerShellEditorServices/blob/master/src/PowerShellEditorServices.Protocol/LanguageServer/References.cs Under the current architecture, the actual request handler will be a method on the It takes in the request type and the arguments, does whatever it needs to do and if required, fires off a response. Again for the response, you have to create a new response type to serialise to. That method is where you'd need to call the relevant API to actually find the occurrences of the variable you were given in the relevant scope (hint: you need the request to tell you not just the variable name but also where it is in the script so you can work out the scope). Language Service APIAll PowerShell-language-related analysis done by the extension currently goes through the AST functionAll the stuff above has just been plumbing. This is where we do the fun, actually-parse-PowerShell-and-help-people part. You need to add some logic that can look at the parsed PowerShell AST, and find the extents ( TestingSince the important part of this is in PSES, you'll want to add xUnit tests somewhere like here that correctly find variable references in some example PowerShell files. Some example tests might be:
Stretch GoalsThis is already plenty of work, and the important part is to get something small working before trying to enhance it, but there are a couple of extra cases you can ignore at first but might be useful to implement at some point (we can kick that out to another PR or just later):
Submitting PRsSince you have to modify two codebases, the general principle is, after any PR is merged neither should have any broken bits. So generally the workflow is:
That should cover most things hopefully, but let us know if you hit any problems. If you get stuck, open a PR, mark it as a WIP and tag us in the comments to let us know you want some guidance |
wow, how could I go wrong with such concise instructions, thanks. Give me a few days to review and get my head around all the working parts. |
If I try and run any of the LanguageService tests I get the following:- Message:
do I need to do anything first? (I've followed step 4 from the readme) |
Bizarre... @Benny1007 are you running the tests in Windows PowerShell or PowerShell Core? |
ah sorry perhaps I should have been a bit clearer. I was just opening the PowerShellEditorServices project in Visual Studio 2017 and running the tests from Test Explorer. |
Ah yes. I personally do not use Visual Studio to run the tests... This could be the problem. Try this in a PowerShell console: Invoke-Build Build Test Assuming you have the @rkeithhill, do you use Visual Studio? Any ideas? |
I used to use the Visual Studio test explorer and that was a real nice experience but that hasn't worked well in a long time (pretty much since we went cross-platform). These days, I use the command line that you refer to. |
Total tests: 194. Passed: 188. Failed: 5. Skipped: 1. |
What does the first give you? Did you try:
|
Yep,
|
I may just delete my fork and re-clone? |
If I update the following lines:- |
@Benny1007 did you try re-cloning? |
yeah that was from the clone |
Sorry, meant to get back sooner. The problem here is that the tests use a hosted PowerShell instance (since xUnit doesn't play well with PowerShell), so it depends on some setup in the build. It looks like you solved that one. The test problems look like those path ones could be test bugs; if you're executing the code from An interim solution might be to try running the build/tests from the |
This is likely a duplicate of #261 |
@Benny1007 @rjmholt Any updates on this feature? I would greatly appreciate if I could rename variables only in their respective scope. |
From the PowerShell team's perspective, currently we are not actively working on this feature due to some other work. However, if anyone (like @Benny1007) does want to pick it back up, we are happy to review PRs. Especially with it being Hacktoberfest :) |
I was under the impression that this would be hard (impossible?) to implement since PowerShell uses
If you were to rename |
Adding onto @rkeithhill's example: & { $foo = 'test' }
. { $foo = 'test' }
& { $foo }
0..10 | ForEach-Object { $foo }
0..10 | Where-Object { $_ -eq $foo }
Invoke-Command -ComputerName dc1 { $foo }
{ $foo } | ForEach-Object { & $_ }
{ $foo } | Out-File ./script.ps1 |
Yes I think our desire here is to implement renaming only in the current scope, so not trying to infer any scope sharing. So if you were to rename |
That could give folks false hope that it worked though... gotta be careful... |
Oh I agree, but at the end of the day we can:
So far we've done (1) and I don't think we should do (3). I get that this is tricky to reason about, and frankly it's hard explaining it to people repeatedly, but there's clearly some demand and there's something we can do and it probably would help in most of the common scenarios. |
As a user, I would be happy if I could rename variables in a function
without worrying that they will be also renamed in other functions just
because the name is the same.
Don't overthink this, it's good enough if variables of the same scope can
be renamed.
…On Sat, Oct 17, 2020, 04:26 Robert Holt ***@***.***> wrote:
Oh I agree, but at the end of the day we can:
1. Give people nothing
2. Implement a safe, restrained feature
3. Make a dangerous decision that does invalid things in various
scenarios
So far we've done (1) and I don't think we should do (3).
I get that this is tricky to reason about, and frankly it's hard
explaining it to people repeatedly, but there's clearly some demand and
there's something we can do and it probably would help in most of the
common scenarios.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1440 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRANOGA7JMNN2VY5QW5HADSLDXC3ANCNFSM4FKOK7DQ>
.
|
@rjmholt Definitely agree that it would be useful. It's hard to guess what the ratio of confusion/frustration to benefit will be though. Most users won't understand the reason why some variables will be renamed and some won't. They may not even know to check if some weren't. I don't really have a problem with trying it out in a preview. Say this gets implemented, which of these would you expect to be renamed? & {
# Rename this
$var = 'thing'
# 1
$var
# 2
0..10 | % { $var }
# 3
{ $var }
# 4
$var = Get-ChildItem
# 5
foreach ($var in (0..10)) {
# 6, 7
$var = 'Number: ' + $var
}
# 8
$var
}
Just to clarify, current scope or current block?
Yeah. Can we think of a good way to alert the user that some variables might not be changed? I worry that we're at best sidegrading problems. Right now if we provide nothing, it doesn't cause problems. But adding it as a feature could end up with folks spending way more time debugging (or worse, end up deleting more than they mean to with a What about adding it as an editor command with the name |
Ugh, I'd really like this too. I think it should just be best-effort. The editor already highlights matched variables, between that and source control I think it's probably enough to mitigate the danger of our scoping issues. |
We're not trying to stop people from breaking code, we're trying to make development more efficient so that you don't stop in the middle of your coding with a ...Huh moment, and don't get interrupted when in the flow. If we replace ALL the instances, in most cases the code actually doesn't break. And it can be argued that is the intent. I have outter variable called X, inner variable I've named X as well, and maybe I want to rename both to Y. Manually fixing by hand the very few edge cases where the inner variable was accidentally named the same often only involved a few lines of code where the entire scope is visible. |
Related on Stack Overflow: PowerShell rename refactor (F2) does nothing in VS Code. |
Seems like this is best approached in a low-hanging fruit process:
The only annoying thing here would be maybe resetting the warning each time we add something which might get some users very annoyed. |
Certainly I work very hard not to reuse var names in PS, and that's pretty easy since lambda functions aren't very common, but a lot of those edge cases like saving a Scriptblock to a variable and executing it in a different scope are more common than you'd think in some tricker modules. As @SeeminglyScience also mentioned we could have a setting for "dangerous renames" which could be more aggressive on renames within the same file only (cross file would be very unlikely to be effective. Which is a greater interrupt of the flow: Your "huh" moment, or spending the next 4 hours trying to figure out why you're suddenly getting a weird error message nowhere near what you were working on only to find the rename grabbed something you completely didn't expect (or worse, finding out at release runtime from users because you didn't have tests). |
It's still an up-for-grabs item so you could go ahead and try a PR, it's on the radar but not committed on anyone's roadmap at the moment. |
@JustinGrote sorry, I think you replied to a comment I deleted. I re-read everyones recent comments and realised they had basically said similar things. (already been covered) My delete and your comment crossed paths, you're just too darned fast! :D |
ooh, what about we create an F# plugin that simply calls ChatGPT API to refactor any code you hilight (as you require), so F2 will pop up a select from preselects. You store your openAPI api key in preferences? When you select the "new" refactoring select choice, you could be presented with a dialog that asks you for title,and chatgpt refactoring prompt. The dialog can also show the "final" prompt that will be sent to chatGPT in a kind of live preview mode, which will most likely consist of some generic prefix (F# project in vscode yada yada context) and suffix (post processing, reconsider the response before responding and if necessary run again ) prompt text etc Just some fun ideas :D (it's trendy and will make bosses love the many productivity options!) I asked ChatGPT, ChatGPT replied, (below) If you press F2 while your cursor is on the topmost $foo in a PowerShell script in an integrated development environment (IDE) that supports refactoring (like Visual Studio Code), and rename it, the IDE will typically attempt to rename all references to $foo within the same scope. In your script, there are different scopes involved. The topmost $foo is declared at the script level. The $foo inside outerFunc is a different variable with the same name but in a different scope. Renaming the topmost $foo would not automatically rename the $foo inside outerFunc, because they are considered separate variables due to their scope. So, if you renamed the topmost $foo to, say, $fooScriptLevel, the IDE should rename the occurrences of $foo in useScriptFoo to $fooScriptLevel, but the $foo inside outerFunc and the $foo referenced in innerFunc (which is the one from outerFunc) should remain unchanged. The script after renaming might look like this: $fooScriptLevel = "bar"
function useScriptFoo {
"script level foo is $fooScriptLevel"
}
function innerFunc {
"outerFunc foo is $foo"
}
function outerFunc {
$foo = "baz" # this is essentially declaring a new variable named foo
innerFunc
}
useScriptFoo
outerFunc
That's very usable I rekon? I can see what @JustinGrote is saying, that the hassle of coding this up using something that parses the actual code, or .net code graphs et al, are seriously non trivial and the effort might be better spent elsewhere?... perhaps like something fun like this? (grin!) |
@goblinfactory feel free to make a separate extension like that, it wouldn't require the PS extension, but it probably will never be in the official extension because:
|
I'm closing this issue in favor of duplicate #261 as it has better details on potential implementation |
F2 doesn't seem to work for PowerShell variables, and the "Rename" option doesn't seem to appear. Renaming PowerShell variables can be tedious, so we should work to support this feature.
But we also need to be careful, since (unlike e.g. C# where lexical scope makes renaming safe) PowerShell's dynamic scoping means that renaming local variable references needs to occur conservatively.
script:
,global:
, andenv:
on the other hand should be easy to rename.The text was updated successfully, but these errors were encountered: