-
Notifications
You must be signed in to change notification settings - Fork 36.3k
feat(ts-codeLens): show "implementations" CodeLens for overridden methods #263749 #264546
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
Conversation
|
Hi team 👋 Local tests ✅ Integration tests ✅ Hygiene checks ✅ |
|
could someone review this please |
|
@ritesh006 We are currently preparing the August release. This PR came in too late to make it in but I've scheduled it for September. Will review it next week when I have a chance |
mjbvz
left a comment
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.
Looks like the code change may have been lost? I only see the new test file and a single new line change
53cc94d to
21cbca8
Compare
|
Thanks for the catch, @mjbvz a merge from main had dropped the provider changes. I’ve restored them and force pushed. The PR now includes the updates in implementationsCodeLens.ts |
jabari-max
left a comment
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'm still only seeing 1 file changed. Can you try again to make sure the other files are included Also no need to explicitly keep copying me |
a2dd3f6 to
85cc11c
Compare
|
Ready for review |
| } | ||
|
|
||
| private getCommand(locations: vscode.Location[], codeLens: ReferencesCodeLens): vscode.Command | undefined { | ||
| if (!locations.length) { |
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.
We still want to show 0 implementations in this case. If you want to avoid showing the code lens for methods with no overrides, that should be handled differently
extensions/typescript-language-features/src/test/smoke/implementationsCodeLens.test.ts
Outdated
Show resolved
Hide resolved
...nsions/typescript-language-features/src/languageFeatures/codeLens/implementationsCodeLens.ts
Outdated
Show resolved
Hide resolved
...nsions/typescript-language-features/src/languageFeatures/codeLens/implementationsCodeLens.ts
Outdated
Show resolved
Hide resolved
5437400 to
2a6a06a
Compare
|
Ready for reviw |
...nsions/typescript-language-features/src/languageFeatures/codeLens/implementationsCodeLens.ts
Outdated
Show resolved
Hide resolved
f1f2365 to
f73e56b
Compare
|
Ready for Review. |
| } | ||
|
|
||
| // Class methods (behind new setting; default off) | ||
| if ( |
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.
What about the other types that were previously in the switch?
3407cb2 to
c0e26f7
Compare
|
Hi @mjbvz ready for review please have a look |
| } | ||
| break; | ||
| if ( | ||
| (item.kind === PConst.Kind.method |
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.
Looks like class got dropped from this list
| parent?.kind === PConst.Kind.class && | ||
| cfg.get<boolean>('implementationsCodeLens.showOnClassMethods', false) | ||
| ) { | ||
| return getSymbolRange(document, item); |
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.
We should skip showing these on private methods as these cannot be overridden
|
HI @mjbvz due to family loss I went to my hometown. as soon as I reach to my place I would like to start work again. |
1202b7b to
c0e26f7
Compare
|
Hi @mjbvz Ready for review |
|
Thanks, looks like there are still a few unresolved comments: https://github.com/microsoft/vscode/pull/264546/files#diff-2c8c54699e981dcd79e99a1b8cbc2690c7b751ac4b160fe7fd3f271835b5b878 Can you please take a look a those Since it's a little late for the September release, we'll also target the October release for getting this merged |
c0e26f7 to
4b77716
Compare
|
Hi @mjbvz Ready for review |
please let me know if anything I missed and so sorry for late reply |
...nsions/typescript-language-features/src/languageFeatures/codeLens/implementationsCodeLens.ts
Show resolved
Hide resolved
...nsions/typescript-language-features/src/languageFeatures/codeLens/implementationsCodeLens.ts
Show resolved
Hide resolved
...nsions/typescript-language-features/src/languageFeatures/codeLens/implementationsCodeLens.ts
Outdated
Show resolved
Hide resolved
...nsions/typescript-language-features/src/languageFeatures/codeLens/implementationsCodeLens.ts
Outdated
Show resolved
Hide resolved
extensions/typescript-language-features/src/test/smoke/implementationsCodeLens.test.ts
Outdated
Show resolved
Hide resolved
67b5546 to
cd5348a
Compare
|
Ready For Review @mjbvz |
|
Added some extra tests to have more confidence in this change. Also renamed it to |
|
@mjbvz red CI |
Summary
This PR adds support for showing "implementations" CodeLens on TypeScript methods that are overridden in derived classes.
Changes
Extended TypeScriptImplementationsCodeLensProvider to surface implementations for:
Class methods (not just interfaces or abstract methods)
Overridden methods in derived classes
Updated extractSymbol logic to correctly identify these symbols
Added smoke test: implementationsCodeLens.test.ts to validate behavior
Why
Resolves #198387.
Developers can now see "1 implementation" / "n implementations" CodeLens above methods that are overridden, and navigate directly to their derived class implementations.
This aligns CodeLens behavior with the “Find All Implementations” command.
Verification
Open an abstract/base class with a method that is overridden in derived classes.
Confirm that the "implementations" CodeLens appears above the method.
Clicking it navigates to the derived class overrides.
Testing
Local build & compile successful
Hygiene checks passed
Smoke test added & executed
Integration tests run successfully (scripts/test-integration.bat)
Notes
This improvement makes CodeLens more consistent and increases productivity when working with object-oriented TypeScript codebases.
Fixes #198387