Skip to content

Fix find all references for salsa#6632

Merged
zhengbli merged 7 commits into
microsoft:masterfrom
zhengbli:fixFindAllRefForSalsa
Jan 28, 2016
Merged

Fix find all references for salsa#6632
zhengbli merged 7 commits into
microsoft:masterfrom
zhengbli:fixFindAllRefForSalsa

Conversation

@zhengbli

@zhengbli zhengbli commented Jan 27, 2016

Copy link
Copy Markdown

Issue:
currently "find all reference" would return no results if called on exported properties in JavaScript file. For example:
before

After the fix:
after

@DanielRosenwasser

DanielRosenwasser commented Jan 27, 2016

Copy link
Copy Markdown
Member

Where are the tests for this change?

@mhegazy

mhegazy commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

looks good. please add some tests.

This was referenced Jan 27, 2016
Closed
Closed
@zhengbli

zhengbli commented Jan 27, 2016

Copy link
Copy Markdown
Author

@DanielRosenwasser @mhegazy @RyanCavanaugh any further comments?

@zhengbli zhengbli mentioned this pull request Jan 27, 2016
Closed
@zhengbli zhengbli mentioned this pull request Jan 27, 2016
Closed
billti
billti reviewed Jan 28, 2016
View reviewed changes
Comment thread src/compiler/checker.ts

@billti billti Jan 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume in most cases it won't be a "specialPropertyAssignmentKind" and you will just fall through here? Can you make this clear in the code by explicitly adding a default: with just a comment to this effect, or wrapping the switch in an if(specialPropertyAssignmentKind...)?

@billti

billti commented Jan 28, 2016

Copy link
Copy Markdown
Member

Added a couple of comments. Looks 👍 to me after that.

DanielRosenwasser
DanielRosenwasser reviewed Jan 28, 2016
View reviewed changes
Comment thread tests/cases/fourslash/renameJsExports.ts Outdated

@DanielRosenwasser DanielRosenwasser Jan 28, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a leading comment for each false

@DanielRosenwasser

DanielRosenwasser commented Jan 28, 2016

Copy link
Copy Markdown
Member

There are no tests for find all refs/rename locs at declaration sites - is this because it doesn't work right now? If it does work, can you add tests?

Either way, can you rename the tests and add a numbered suffix?i.e.renameJsExports.tsbecomesrenameJsExports01.ts`). I can see us adding more tests in the future that test renames on exports within a JS file.

zhengbli pushed a commit that referenced this pull request Jan 28, 2016
Merge pull request #6632 from zhengbli/fixFindAllRefForSalsa
31bf103 
Fix find all references for salsa
@zhengbli zhengbli merged commit 31bf103 into microsoft:master Jan 28, 2016
@zhengbli zhengbli deleted the fixFindAllRefForSalsa branch January 28, 2016 19:44
@billti billti mentioned this pull request Feb 4, 2016
Merged
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zhengbli @DanielRosenwasser @mhegazy @billti @msftclas