-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Prioritize case
and default
keywords in switch statement completions
#62126
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: DanielRosenwasser <[email protected]>
//// /*newPrio3*/ | ||
//// } | ||
|
||
//// // NEW - prioritize case/default (after throw) |
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.
Remove these NEW labels, don't claim what to expect. Instead, describe the positioning of these.
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.
Removed all "NEW" labels and improved the descriptions to focus on positioning rather than claiming expectations. Comments now describe the context (e.g., "After break statement with proper indentation") instead of predicting behavior. Fixed in 7f7a30e.
//// /*newPrio4*/ | ||
//// } | ||
|
||
verify.completions( |
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.
Switch this to baselining
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.
Switched to baseline testing approach using verify.baselineCompletions() for all markers instead of explicit verify.completions() assertions. The baseline now clearly shows case/default prioritization (sortText "10") in appropriate contexts and normal ordering (sortText "15") elsewhere. Fixed in 7f7a30e.
case
and default
in a switch
statementcase
and default
keywords in switch statement completions
Co-authored-by: DanielRosenwasser <[email protected]>
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.
The diff is huge, so maybe this needs to run with noLib
or some similar thing to trim down the full completion list.
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 @noLib: true
and switched from baseline testing to targeted verify.completions()
assertions. This reduced the test from 256k+ lines to a focused test that validates the specific sortText values ("10" for prioritized, "15" for normal) without the massive global completion list. Fixed in c8b6d8c.
@typescript-bot pack this |
@typescript-bot Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@typescript-bot Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Co-authored-by: DanielRosenwasser <[email protected]>
verify.completions({ | ||
includes: [ | ||
{ name: "case", sortText: "10" }, | ||
{ name: "default", sortText: "10" } |
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.
I think it should be tested that thing
is still in the completion list.
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 explicit verification that thing
is still in the completion list with proper sortText ("11") across all test scenarios. The test now uses both baseline testing to show relative ordering and targeted verify.completions()
to confirm thing
availability. Fixed in 75fcd1d.
}); | ||
|
||
goTo.marker("newPrio1"); | ||
verify.completions({ |
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.
Go back to baselining if the baselines aren't massive, and show some characteristics about the ordering. The key thing the tests need to witness is whether case
, default
, and thing
appear in the list and what their relative order would be.
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.
Switched back to baseline testing with @nolib to keep baselines manageable (~548KB). The baseline now clearly shows the relative ordering of case
, default
, and thing
in different contexts:
- Prioritized contexts:
case
/default
sortText "10",thing
sortText "11" - Non-prioritized contexts:
case
/default
sortText "15",thing
sortText "11"
Fixed in 75fcd1d.
…ordering and verify thing availability Co-authored-by: DanielRosenwasser <[email protected]>
This PR implements smart prioritization of
case
anddefault
keywords in TypeScript switch statement completions, addressing a longstanding usability issue where these essential keywords appeared buried in the completion list.Problem
Previously, when requesting completions inside a switch statement,
case
anddefault
keywords used the default sort order, causing them to appear after other completions:This was problematic because these keywords are almost always what developers need in switch contexts, but they were sorted alphabetically after other suggestions.
Solution
The implementation adds intelligent context detection to prioritize
case
anddefault
keywords when:Implementation Details
shouldPrioritizeCaseAndDefaultKeywords()
detects switch contextsSortText.LocalDeclarationPriority
("10") instead ofSortText.GlobalsOrKeywords
("15")Testing
Added comprehensive test coverage in
switchCaseCompletionPriority.ts
using baseline testing to demonstrate the relative ordering ofcase
,default
, and other completions:case
/default
with sortText "10"case
/default
with sortText "15"thing
remain available with consistent sortText "11"Fixes #52234.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.