Skip to content

Add properties priority for completion - #32266

Merged
Sheetal Nandi (sheetalkamat) merged 8 commits into
microsoft:masterfrom
fuafa:properties-priorities
Sep 5, 2019
Merged

Add properties priority for completion#32266
Sheetal Nandi (sheetalkamat) merged 8 commits into
microsoft:masterfrom
fuafa:properties-priorities

Conversation

@fuafa

@fuafaZixiang Li (fuafa) commented Jul 5, 2019

Copy link
Copy Markdown
Contributor

Fixes#29868

sheetalkamat
Sheetal Nandi (sheetalkamat) reviewed Jul 11, 2019
View reviewed changes
Comment threadsrc/services/completions.ts Outdated
sheetalkamat
Sheetal Nandi (sheetalkamat) requested changes Jul 11, 2019
View reviewed changes

@sheetalkamatSheetal Nandi (sheetalkamat) left a comment

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.

Thank you for working on this.

Comment threadsrc/services/completions.ts Outdated
Comment threadsrc/harness/fourslash.ts Outdated
sheetalkamat
Sheetal Nandi (sheetalkamat) reviewed Jul 16, 2019
View reviewed changes
Comment threadsrc/services/completions.ts Outdated
sheetalkamat
Sheetal Nandi (sheetalkamat) requested changes Jul 18, 2019
View reviewed changes
Comment threadsrc/services/completions.ts Outdated
sheetalkamat
Sheetal Nandi (sheetalkamat) requested changes Jul 18, 2019
View reviewed changes
Comment threadsrc/services/completions.ts Outdated
Comment threadsrc/services/completions.ts Outdated
Comment threadsrc/services/completions.ts Outdated

// Set SortText to MemberDeclaredBySpreadAssignment if it is fulfilled by spread assignment
function setSortTextToMemberDeclaredBySpreadAssignment(membersDeclaredBySpreadAssignment: Symbol[], contextualMemberSymbols: Symbol[]): void {
for (const fulfilledSymbol of membersDeclaredBySpreadAssignment) {

@sheetalkamatSheetal Nandi (sheetalkamat)Jul 18, 2019

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.

Short circuit for filteredSymbols .length === 0 is needed.

Comment threadsrc/services/completions.ts Outdated
Comment threadsrc/services/completions.ts Outdated
sheetalkamat
Sheetal Nandi (sheetalkamat) reviewed Jul 25, 2019
View reviewed changes
Comment threadsrc/services/completions.ts Outdated

if (isBindingElement(m) && m.propertyName) {
if (isSpreadAssignment(m)) {
const expression = m.expression;

@sheetalkamatSheetal Nandi (sheetalkamat)Jul 25, 2019

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.

Ok last one change needed.
Scope out this login into another function that sets the members in map so that it can be shared for object members and jsx attributes

@sheetalkamat
Sheetal Nandi (sheetalkamat) dismissed their stale reviewJuly 25, 2019 19:56

approved with last comment to fix

@fuafa

Zixiang Li (fuafa) commented Aug 13, 2019

Copy link
Copy Markdown
ContributorAuthor

Anything I miss? Daniel Rosenwasser (@DanielRosenwasser)Sheetal Nandi (@sheetalkamat) 🆙 🚀

sheetalkamat
Sheetal Nandi (sheetalkamat) reviewed Aug 13, 2019
View reviewed changes
Comment threadsrc/services/completions.ts Outdated
return contextualMemberSymbols;
}

const membersDeclaredBySpreadAssignment = createMap<boolean>();

@sheetalkamatSheetal Nandi (sheetalkamat)Aug 13, 2019

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 needs to be Map since we don't use actual value.

@fuafaZixiang Li (fuafa)Aug 15, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

But it is a Map already, and what do you mean we don't use actual value?

@sheetalkamatSheetal Nandi (sheetalkamat)Aug 19, 2019

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 was on mobile and didnt realise its not showing correctly. Sorry about that. It needs to be Map<true>

@fuafaZixiang Li (fuafa)Aug 19, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks! Fixed.

sheetalkamat
Sheetal Nandi (sheetalkamat) reviewed Aug 19, 2019
View reviewed changes
Comment threadsrc/services/completions.ts Outdated
}

// Set SortText to MemberDeclaredBySpreadAssignment if it is fulfilled by spread assignment
function setSortTextToMemberDeclaredBySpreadAssignment(membersDeclaredBySpreadAssignment: Map<boolean>, contextualMemberSymbols: Symbol[]): void {

@sheetalkamatSheetal Nandi (sheetalkamat)Aug 19, 2019•
edited
Loading

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.

Change to Map<true> here too.

@sheetalkamat
Sheetal Nandi (sheetalkamat) merged commit c26c44d into microsoft:masterSep 5, 2019
@fuafa
Zixiang Li (fuafa) deleted the properties-priorities branch September 7, 2019 21:05
@timsuchanekTim Suchanek (timsuchanek) mentioned this pull request Sep 11, 2019
Closed
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasserDaniel Rosenwasser (DanielRosenwasser)DanielRosenwasser left review comments
@sheetalkamatSheetal Nandi (sheetalkamat)sheetalkamat approved these changes

Assignees

@sheetalkamatSheetal Nandi (sheetalkamat)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Optional and already-fulfilled properties should have lower priorities than required properties

3 participants

@fuafa@DanielRosenwasser@sheetalkamat