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

Limit inline values to current function-scope #14

Merged
merged 15 commits into from
Nov 3, 2022

Conversation

fflaten
Copy link
Contributor

@fflaten fflaten commented Jun 12, 2022

The extension only shows values for variables in local scope. To reduce noise and wrong values (local values shown for matching variables in parent scopes) this PR filters the lines that are being scanned to only cover relevant parts.

Use function start as startline when stopped inside a function, else document start as before. Always exclude other functions.

  • Setting to control behavior
    • Remove? Unnecessary?
  • Calculate startLine (document or function start) depending on stopped location
  • Always exclude lines for functions not in scope
  • Add tests
  • Test for performance impact and optimize. Especially for large files (tested against Pester.psm1 ~17k lines)

Fix #8

@fflaten
Copy link
Contributor Author

fflaten commented Jun 13, 2022

@TylerLeonhardt Any way to profile this type of extension? Start Extension Host Profile doesn't show the methods in this code. Because it's a provider? Or because of async?

@fflaten fflaten marked this pull request as ready for review June 14, 2022 19:10
@fflaten
Copy link
Contributor Author

fflaten commented Jun 14, 2022

This is ready for review.

  • Configuration option. Text, name etc. ok?
    • Should it even be a setting considering how many wrong answers that can be shown in parent scopes using document?
  • Any tips for general naming and structure. Both for code and tests?
  • Optimizations?
  • Make get* functions public + unit tests?
  • How to apply MIT license properly for borrowed code in testUtils.ts ?

@TylerLeonhardt
Copy link
Owner

I noticed you had a couple other commits. Ready for me now @fflaten?

@fflaten
Copy link
Contributor Author

fflaten commented Jun 27, 2022

Yes please. Ready for review, but not merge as I'm missing a MIT license if not more depending on comments.

Glad I did the last commits while I waited though as it went from 11 to 1 sec test time 😄

@fflaten
Copy link
Contributor Author

fflaten commented Oct 31, 2022

Friendly reminder for review 🙂

src/documentParser.ts Show resolved Hide resolved
@TylerLeonhardt TylerLeonhardt reopened this Nov 3, 2022
@TylerLeonhardt TylerLeonhardt merged commit 65aafa5 into TylerLeonhardt:master Nov 3, 2022
@TylerLeonhardt
Copy link
Owner

Awesome work! Published. It should take a few minutes to show up

@fflaten
Copy link
Contributor Author

fflaten commented Nov 4, 2022

Thanks 👏
Did you have any thoughts on the questions above? Like if the setting is neccessary at all?

If PowerShell/PowerShellEditorServices#1886 gets merged it would be natural to update this to also track/scope to class, configuration or method/constructor in addition to functions.

@fflaten fflaten deleted the function-view branch November 4, 2022 12:40
@TylerLeonhardt
Copy link
Owner

I think leaving the setting there for now is totally ok. Let's see if anyone complains about the new behavior and if they do, they have an easy way to revert back. If no one complains, then we can take it out :)

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.

Feature request: Only show inline values inside the current scope
2 participants