-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Fix(52604): Provide Object member completions without comma; insert a comma #52899
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
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
7c72784
Add condition to fix without adding comma
navya9singh fa517aa
Fix for adding comma
navya9singh 6613dca
fixing test and adding checks for other tests
navya9singh 4f88134
removing commented code
navya9singh 586a2e5
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh 3ab385f
Adressing pr comments
navya9singh abb5c07
fixing auto-imports
navya9singh c8c1362
fixing lint error
navya9singh 463a435
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh 54456cc
Adressing pr comments and adding test cases
navya9singh 87b09f8
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh 2fff618
adding test cases
navya9singh b33aef8
Minor changes and adding a test case
navya9singh 731222c
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh 89d178b
Apply suggestions from code review
DanielRosenwasser 790b30c
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
navya9singh c477b17
Merge branch 'fix(52604)' of https://github.com/microsoft/TypeScript …
navya9singh 7e861b6
Adding test for spread assignment
navya9singh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
43 changes: 43 additions & 0 deletions
43
tests/cases/fourslash/completionsObjectLiteralExpressions1.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
/// <reference path="fourslash.ts" /> | ||
//// interface ColorPalette { | ||
//// primary?: string; | ||
//// secondary?: string; | ||
//// } | ||
|
||
//// let colors: ColorPalette = { | ||
//// primary: "red" | ||
//// /**/ | ||
//// }; | ||
|
||
verify.completions({ | ||
marker: "", | ||
includes: [{ | ||
name: "secondary", | ||
sortText: completion.SortText.OptionalMember, | ||
hasAction: true, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
}], | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); | ||
|
||
verify.applyCodeActionFromCompletion("", { | ||
name: "secondary", | ||
description: `Add missing comma for object member completion 'secondary'.`, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
newFileContent: | ||
`interface ColorPalette { | ||
primary?: string; | ||
secondary?: string; | ||
} | ||
let colors: ColorPalette = { | ||
primary: "red", | ||
|
||
};`, | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); |
47 changes: 47 additions & 0 deletions
47
tests/cases/fourslash/completionsObjectLiteralExpressions10.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/// <reference path="fourslash.ts" /> | ||
//// interface TTTT { | ||
//// aaa: string, | ||
//// bbb?: number | ||
//// } | ||
//// const uuu: TTTT = { | ||
//// get aaa() { | ||
//// return "" | ||
//// } | ||
//// /**/ | ||
//// } | ||
|
||
verify.completions({ | ||
marker: "", | ||
includes: [{ | ||
name: "bbb", | ||
sortText: completion.SortText.OptionalMember, | ||
hasAction: true, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
}], | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); | ||
|
||
verify.applyCodeActionFromCompletion("", { | ||
name: "bbb", | ||
description: `Add missing comma for object member completion 'bbb'.`, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
newFileContent: | ||
`interface TTTT { | ||
aaa: string, | ||
bbb?: number | ||
} | ||
const uuu: TTTT = { | ||
get aaa() { | ||
return "" | ||
}, | ||
|
||
}`, | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); | ||
|
48 changes: 48 additions & 0 deletions
48
tests/cases/fourslash/completionsObjectLiteralExpressions2.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/// <reference path="fourslash.ts" /> | ||
//// interface ColorPalette { | ||
//// primary?: string; | ||
//// secondary?: string; | ||
//// } | ||
|
||
//// interface I { | ||
//// color: ColorPalette; | ||
//// } | ||
|
||
//// const a: I = { | ||
//// color: {primary: "red" /**/} | ||
//// } | ||
|
||
verify.completions({ | ||
marker: "", | ||
includes: [{ | ||
name: "secondary", | ||
sortText: completion.SortText.OptionalMember, | ||
hasAction: true, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
}], | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
} | ||
}); | ||
|
||
verify.applyCodeActionFromCompletion("", { | ||
name: "secondary", | ||
description: `Add missing comma for object member completion 'secondary'.`, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
newFileContent: | ||
`interface ColorPalette { | ||
primary?: string; | ||
secondary?: string; | ||
} | ||
interface I { | ||
color: ColorPalette; | ||
} | ||
const a: I = { | ||
color: {primary: "red", } | ||
}`, | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); |
45 changes: 45 additions & 0 deletions
45
tests/cases/fourslash/completionsObjectLiteralExpressions3.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/// <reference path="fourslash.ts" /> | ||
////interface T { | ||
//// aaa?: string; | ||
//// foo(): void; | ||
//// } | ||
//// const obj: T = { | ||
//// foo() { | ||
// | ||
//// } | ||
//// /**/ | ||
//// } | ||
|
||
verify.completions({ | ||
marker: "", | ||
includes: [{ | ||
name: "aaa", | ||
sortText: completion.SortText.OptionalMember, | ||
hasAction: true, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
}], | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); | ||
|
||
verify.applyCodeActionFromCompletion("", { | ||
name: "aaa", | ||
description: `Add missing comma for object member completion 'aaa'.`, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
newFileContent: | ||
`interface T { | ||
aaa?: string; | ||
foo(): void; | ||
} | ||
const obj: T = { | ||
foo() { | ||
}, | ||
|
||
}`, | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); |
42 changes: 42 additions & 0 deletions
42
tests/cases/fourslash/completionsObjectLiteralExpressions4.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/// <reference path="fourslash.ts" /> | ||
////interface T { | ||
//// aaa: number; | ||
//// bbb?: number; | ||
//// } | ||
//// const obj: T = { | ||
//// aaa: 1 * (2 + 3) | ||
//// /**/ | ||
//// } | ||
|
||
verify.completions({ | ||
marker: "", | ||
includes: [{ | ||
name: "bbb", | ||
sortText: completion.SortText.OptionalMember, | ||
hasAction: true, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
}], | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); | ||
|
||
verify.applyCodeActionFromCompletion("", { | ||
name: "bbb", | ||
description: `Add missing comma for object member completion 'bbb'.`, | ||
source: completion.CompletionSource.ObjectLiteralMemberWithComma, | ||
newFileContent: | ||
`interface T { | ||
aaa: number; | ||
bbb?: number; | ||
} | ||
const obj: T = { | ||
aaa: 1 * (2 + 3), | ||
|
||
}`, | ||
preferences: { | ||
allowIncompleteCompletions: true, | ||
includeInsertTextCompletions: true, | ||
}, | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I've been thinking about the difference between
contextToken
andpreviousToken
andlocation
, and when to use each one, and I found another case that doesn't seem to be working:I have a feeling that we might want to be using
previousToken
instead ofcontextToken
, but I might be wrong.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is not a new test case, it's the same as this one I previously commented:
The relevant factor seems to be that there is an identifier in the right side of the property assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC,
The idea is in most places, the completions should be the same whether or not you’ve already started typing something at the current location, so
contextToken
usually provides a stable token between those two similar cases, wherepreviousToken
changes. But I could be oversimplifying that, and the typical logic of “just usecontextToken
” may not apply when checking to see if there’s a comma. But there should definitely be tests both where the beginning of the expected completion has already been typed and where nothing has been typed yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test cases to check when the beginning of the expected completion is typed.