Skip to content

Fix rename/document-highlight/find reference in parameter property declaration#6084

Merged
yuit merged 22 commits into
masterfrom
fix4686_fixrename
Dec 18, 2015
Merged

Fix rename/document-highlight/find reference in parameter property declaration#6084
yuit merged 22 commits into
masterfrom
fix4686_fixrename

Conversation

@yuit
Copy link
Copy Markdown
Contributor

@yuit yuit commented Dec 13, 2015

fix #4686 rename/document highlighting/ find reference in parameter property declaration.

Comment thread src/compiler/binder.ts Outdated
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.

isParameterPropertyDeclaration

Comment thread Jakefile.js Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is currently only services file as to fix entire services folder for linting will be a large fix that is not necessary have to be done in one-go

Comment thread src/compiler/checker.ts
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 wonder if the return type here should make it explicit over which is which (i.e.{ parameterSymbol: Symbol; propertySymbol: Symbol; }). Perhaps that's overkill. Thoughts @vladima, @CyrusNajmabadi?

@DanielRosenwasser
Copy link
Copy Markdown
Member

I can't see any of the changes in services.ts due to the linting introduction. I think that change should have been done separately from this PR, so can we temporarily remove that change and cherry pick your lint fixes into a separate PR?

@yuit
Copy link
Copy Markdown
Contributor Author

yuit commented Dec 15, 2015

@DanielRosenwasser yes I can do that. I forgot that github can't show diff of too many change 😅

Comment thread src/services/services.ts
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 wonder if it's permissible to repeat the same symbol, but I think it should be fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How can it be the same symbol though?

@DanielRosenwasser
Copy link
Copy Markdown
Member

👍

Comment thread src/harness/fourslash.ts Outdated
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.

This should be assertHasRanges

yuit added a commit that referenced this pull request Dec 18, 2015
Fix rename/document-highlight/find reference in parameter property declaration
@yuit yuit merged commit 09d2233 into master Dec 18, 2015
@yuit yuit deleted the fix4686_fixrename branch December 18, 2015 05:52
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to rename parameter properties

3 participants