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

Variable symbols #15

Closed
wants to merge 3 commits into from
Closed

Variable symbols #15

wants to merge 3 commits into from

Conversation

lgabeskiria
Copy link
Contributor

We should exclude variable symbols that are defined in function and method body.

@codecov-io
Copy link

Current coverage is 89.94% (diff: 100%)

Merging #15 into master will increase coverage by 0.38%

@@             master        #15   diff @@
==========================================
  Files            14         14          
  Lines           182        189     +7   
  Methods          29         30     +1   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            163        170     +7   
  Misses           19         19          
  Partials          0          0          
Diff Coverage File Path
•••••••••• 100% src/SymbolFinder.php

Powered by Codecov. Last update 7138088...1502c8b

@felixfbecker
Copy link
Owner

felixfbecker commented Sep 8, 2016

Should we? I remember that TypeScript language server once behaved that way, and they changed the behavior: microsoft/TypeScript#8218

@lgabeskiria
Copy link
Contributor Author

I have latest version of typescript and it does not show symbols inside functions.

From my point of view it's better to hide symbols inside functions.
You can open some big php file and you'll see that it's quite uncomfortable to navigate to other method in class when you have variable symbols.

@lgabeskiria
Copy link
Contributor Author

Alternatively we can add option to enable/disable this behavior.

@felixfbecker
Copy link
Owner

I double checked and you are right, we should follow TS' behavior here. A setting may be nice for a future update, but low priority.

@felixfbecker
Copy link
Owner

Could you rebase this on master (so it only includes the actual code changes)?

@lgabeskiria
Copy link
Contributor Author

yes.

@felixfbecker
Copy link
Owner

To prevent this in the future, do you have a recommendation for #12 ?

@lgabeskiria
Copy link
Contributor Author

I was unable to rebase the branch so I created another pull request #16 .
you can close this one.

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