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

Fix finding variable definitions in workspace #458

Merged
merged 4 commits into from
May 19, 2017

Conversation

kapilmb
Copy link

@kapilmb kapilmb commented May 19, 2017

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Looks good! I wonder if there's any special consideration we need to take for $module:-scope variables?

@@ -14,25 +14,31 @@ namespace Microsoft.PowerShell.EditorServices
internal class FindDeclartionVisitor : AstVisitor
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind fixing the typo in the name of this class? FindDeclarationVisitor. That's bugged me for a while :)

Copy link
Author

Choose a reason for hiding this comment

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

fixed!

@kapilmb
Copy link
Author

kapilmb commented May 19, 2017

To take scope into account we have two options -

  1. implement our own ad-hoc logic OR
  2. use the variable analysis part in PSSA.

The advantage with 2 is that we will have a robust solution and the techniques used there can then also be used in rename logic.

@daviwil
Copy link
Contributor

daviwil commented May 19, 2017

I'm all for using the existing variable analysis logic, maybe it's better to wait to do that until we can centralize our analysis and workspace logic in a reusable module

@kapilmb
Copy link
Author

kapilmb commented May 19, 2017

The existing variable analysis logic is still somewhat buggy and needs some work. But I agree that we should centralize our analysis logic then use it from there.

@daviwil
Copy link
Contributor

daviwil commented May 19, 2017

Thanks dude! Feel free to merge it once the tests pass

@kapilmb kapilmb merged commit 6cdd65b into PowerShell:master May 19, 2017
@kapilmb kapilmb deleted the fix-variable-definition branch May 19, 2017 03:30
@daviwil daviwil modified the milestone: 1.2.0 May 19, 2017
TylerLeonhardt pushed a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Feb 26, 2019
First phase of streamlined development setup
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.

3 participants