Skip to content

Get occurrences for 'this' keywords#600

Merged
DanielRosenwasser merged 4 commits into
getOccurrencesfrom
getOccurrencesThis
Sep 9, 2014
Merged

Get occurrences for 'this' keywords#600
DanielRosenwasser merged 4 commits into
getOccurrencesfrom
getOccurrencesThis

Conversation

@DanielRosenwasser
Copy link
Copy Markdown
Member

No description provided.

Comment thread src/services/services.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems odd. Why not have a getThisContainer function instead? and if someone wants a thisContainer or arrowFunction we can have a helper for that (which just uses a common helper).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The thing is that the original uses all needed to track if an arrow function was encountered. I agree, I may just simplify the logic and make that getThisContainer function which performs the do/while logic.

Comment thread src/compiler/parser.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather than defining two functions, one of which has two nested loops, I would just take a parameter for whether to include arrow functions.

Comment thread src/services/services.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will break rename, allowing you to rename "this".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh true!!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i would remove this for now. Until we're ready to decide what it means to both findrefs on 'this' as well as what 'rename this' means.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ahejlsberg and I felt like renaming this isn't so unheard of, but yes, it is currently broken in this implementation - rename will populate the initial text with the symbol name in VS.

What semantics did you have for findAllRefs on this? While we don't go out of our way to match this across merged declarations, we certainly have fairly predictable results. Thoughts @mhegazy, as we discussed this offline?

DanielRosenwasser added a commit that referenced this pull request Sep 9, 2014
Get occurrences for 'this' keywords
@DanielRosenwasser DanielRosenwasser merged commit dd5d216 into getOccurrences Sep 9, 2014
@DanielRosenwasser DanielRosenwasser deleted the getOccurrencesThis branch September 11, 2014 22:59
@DanielRosenwasser DanielRosenwasser restored the getOccurrencesThis branch September 11, 2014 22:59
@DanielRosenwasser DanielRosenwasser deleted the getOccurrencesThis branch September 11, 2014 22:59
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants