X Tutup
The Wayback Machine - https://web.archive.org/web/20221217192153/https://github.com/microsoft/TypeScript/pull/51236
Skip to content
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

Jump to bottom

fix(51225): Go-to-definition on case or default should jump to the containing switch statement if available. #51236

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(51225): Go-to-definition on case or default should jump to the containing switch statement if available. #51236

wants to merge 3 commits into from

Conversation

sviat9440
Copy link

@sviat9440 sviat9440 commented Oct 20, 2022

Fixes #51225

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 20, 2022
@sviat9440
Copy link
Author

sviat9440 commented Oct 20, 2022

@microsoft-github-policy-service agree

@sviat9440 sviat9440 force-pushed the fix/51225 branch 2 times, most recently from 1613057 to 7b3a71a Compare Oct 20, 2022
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 20, 2022

Can you add one more test?

switch (null) {
    case null:
        export /*start*/default 123;
}

Validate that it doesn't jump to the switch statement.

@sviat9440
Copy link
Author

sviat9440 commented Oct 20, 2022 •
edited 

/////*c*/export default { [|/*a*/case|] };
////[|/*b*/default|];

Is it normal that b and goes to c? This is how it works now and looks like a bug. It is expected that there will be no go to anywhere.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 20, 2022

It's questionable, but I wouldn't get hung up on fixing that specific case.

@sviat9440 sviat9440 force-pushed the fix/51225 branch 3 times, most recently from 256cc27 to 26decab Compare Oct 21, 2022
@sviat9440 sviat9440 requested review from RyanCavanaugh and DanielRosenwasser and removed request for RyanCavanaugh and DanielRosenwasser Oct 21, 2022
@sviat9440 sviat9440 mentioned this pull request Oct 21, 2022
Open
sheetalkamat
sheetalkamat requested changes Oct 21, 2022
View changes
src/services/goToDefinition.ts Outdated Show resolved Hide resolved
src/services/goToDefinition.ts Outdated Show resolved Hide resolved
src/services/goToDefinition.ts Outdated Show resolved Hide resolved
src/services/goToDefinition.ts Outdated Show resolved Hide resolved
src/services/goToDefinition.ts Outdated Show resolved Hide resolved
@sviat9440 sviat9440 requested review from sheetalkamat and removed request for DanielRosenwasser Oct 22, 2022
@sviat9440 sviat9440 force-pushed the fix/51225 branch 3 times, most recently from d3e1701 to d7575ab Compare Nov 2, 2022
@sviat9440 sviat9440 requested review from DanielRosenwasser and removed request for sheetalkamat Nov 2, 2022
@sviat9440 sviat9440 requested a review from DanielRosenwasser Nov 2, 2022
@sviat9440 sviat9440 requested a review from sheetalkamat Nov 2, 2022
sheetalkamat
sheetalkamat reviewed Nov 2, 2022
View changes
Copy link
Member

@sheetalkamat sheetalkamat left a comment

This looks good but the tests dont verify contextSpan is coming out correct.

Also should context be switch keyword plus expression or just expression.. Probably expression is fine but i am not sure if editor works out ok if context span is mutually exclusive with definition span.. (dont think we have had that)

@sandersn sandersn added this to Not started in PR Backlog Nov 14, 2022
@sandersn sandersn moved this from Not started to Waiting on author in PR Backlog Nov 14, 2022
sviat9440 added 2 commits Nov 24, 2022
@sviat9440
Merge branch 'main' into fix/51225
1c1599f 
# Conflicts:
#	src/services/goToDefinition.ts
@sviat9440
fix(51225): Fix context span
1995f33
@sviat9440
Copy link
Author

sviat9440 commented Nov 24, 2022 •
edited

@sheetalkamat I changed contextSpan to keyword + expression, but i can't find any examples for testing contextSpan with fourslash

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Dec 9, 2022

Good to go @sheetalkamat?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanCavanaugh RyanCavanaugh

@sheetalkamat sheetalkamat

@DanielRosenwasser DanielRosenwasser

Assignees

@sheetalkamat sheetalkamat

Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Waiting on author
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Go-to-definition on case or default should jump to the containing switch statement if available.
5 participants
@sviat9440 @DanielRosenwasser @RyanCavanaugh @sheetalkamat @typescript-bot
X Tutup