-
Notifications
You must be signed in to change notification settings - Fork 5
Add support for expressions #7
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
base: master
Are you sure you want to change the base?
Conversation
Got a bit excited about the expression idea and played with it. I'm not familiar with typescript/javascript development, so this is only a proof-of-concept and most likely not best practice 😄 . Missing tests atm. Pros:
Cons:
|
@fflaten technically it is possible for a property getter to change state. class Person
{
private string name; // field
private int i;
public string Name // property
{
get { i++; return name; } // get method
set { name = value; } // set method
}
} It's horrible and no one should do this... but it is possible. And since it's possible, we'll need setting to control enabling or disabling this feature. I think the feature can be on by default. Let me know if you need help with that part but the docs on configuration can be found here: https://code.visualstudio.com/api/references/contribution-points#contributes.configuration |
Generally, I think what you may want to do is find all of the variables first.... then use their indexes in the file to see if they have a property. That way, instead of one regex that does everything, it's 2 regexes:
This will be easier to write a setting for too, I think. |
Absolutely. This is actually a "problem"/risk with vscode / vscode-powershell too. If you expand the variable in the variable explorer and step over a line the update of the variable explorer view will trigger the same getter/scriptproperty. With this PoC-feature enabled, we potentially call this bad code twice on each step. Good call on the setting - a must-have if this feature is implemented. Still on the fence whether or not this PR is worthwhile with these risks. Another thing is that I originally assumed that vscode would use the range-value to keep track and only update inlinevalues as you step over them (including reruns for lines in a loop). That's not the case. On one hand updating previous inlinevalues are useful for easy visibility, but at the same time it is almost rewriting history. Showing current values are expected in the variable explorer, but scrolling up to previously executed code and seeing inlinevalues with a suddenly changed value can be confusing for users who might reuse variable names in the same local scope.
Not sure about how it would affect the setting, but it would keep the code cleaner for variable-only scenarios. 👍 |
Time for an update here. I haven't completely abandoned this PR and it wasn't the configuration-option that scared me. 😄 Also considering #8 it would be very helpful to have access to AST so we can make smarter decisions. Is there any way to access a PowerShell-parser in this extension? Or execute commands in the debugger/PSES session? There's no relevant external APIs provided by vscode-powershell to hook into and AFAIK there's no support for any "evaluate/execute" command we can pass to the debugger (or is it?). My current ideas are:
As I'm not experienced in neither JS/TS or vscode extension development I might be missing something here, so please help 🙂 If not, then I'm wondering if it's time to consider moving this feature inside the official vscode-powershell extension where all of this might be easier to implement. I believe this awesome extension has gotten enough popularity and showed it's usefulness to be included as a first-class feature for PowerShell in VSCode just like it is in the Java-extensions. |
@fflaten tl;dr: Yes this should use PSES. However, I don't think it should use PSES but instead an API provided by
or like you said, it becomes a part of the vscode-powershell extension. Both of these require some work from @andschwa or @rjmholt on the vscode-powershell side to enable us to do this. We could contribute it to vscode-powershell, but right now the current focus of the PowerShell team is on performance of PSES and I don't think adding more complexity to it is what we wanna do right this second. I think this extension can continue being regex based for the time being and in the future we can work with the PowerShell team on a solution that makes sense. With all that said, if you wanted to contribute:
I think there's a ton of value there since the vscode-powershell stuff won't happen for a while most likely. |
Couldn't agree more. I do support prioritizing PSES performance and stability in vscode-powershell, so I guess we'll have to be patient about getting this integrated or get access to a suitable external API at some point. Hopefully it's not years away 🙂 Will look into finishing this PR with regex when I get some time. |
Experimenting with support for expressions in addition to variables.
Fix #3
()