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

Add container information to CallHierarchyItem #37061

Closed
mjbvz opened this issue Feb 26, 2020 · 8 comments · Fixed by #38997 or #40348
Closed

Add container information to CallHierarchyItem #37061

mjbvz opened this issue Feb 26, 2020 · 8 comments · Fixed by #38997 or #40348
Assignees
Labels
API Relates to the public API for TypeScript Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Feb 26, 2020

From microsoft/vscode#89616

Search Terms

  • call hierarchy
  • tsserver

Problem

Currently using the call hierarchy api, it can be difficult to browse through long list of callers and understand where those callers are:

Screen Shot 2020-02-26 at 3 12 17 PM

Suggestion

Add optional information about the container on the CallHierarchyItem type. For example, the NavTooItem has:

containerName?: string;
containerKind?: ScriptElementKind;

We should consider something similar for CallHierarchyItem

/cc @jrieken

@DanielRosenwasser DanielRosenwasser added API Relates to the public API for TypeScript Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Suggestion An idea for TypeScript labels Feb 27, 2020
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Feb 27, 2020

Given that you're in call hierarchy, won't you end up with many of the same icons anyway? What sorts of distinctions would be helpful here, and would they only apply to methods on classes?

@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Feb 27, 2020
@mjbvz
Copy link
Contributor Author

mjbvz commented Feb 27, 2020

Yes I don't think we would use the containerKind to display an icon. It may be helpful to understand the type of the callee for example

@mjbvz
Copy link
Contributor Author

mjbvz commented Feb 27, 2020

@jrieken Do you think we should show the file name for every entry?

Screen Shot 2020-02-26 at 5 22 03 PM

This is similar to workspace symbol search results

@jrieken
Copy link
Member

jrieken commented Feb 27, 2020

In a very first version we showed the file name and that was disliked - often different (contained) types are in the same file. Also file paths are very long.

That's why container name is wanted, which sometimes might be a file name but often is a class or namespace name. Having this will provide much more context and will make it easier to work with the call hierarchy (which by its nature can be confusing already)

@mjbvz mjbvz added the VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone label Apr 9, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.0 milestone Apr 15, 2020
@mjbvz
Copy link
Contributor Author

mjbvz commented Apr 15, 2020

Here's my proposal for the TS Server api:

interface CallHierarchyItem {
    /**
      * Name of symbol's container symbol (if any), such as the class name.
     */
    containerName?: string;
}

Not sure we have a use for containerKind so I'm leaving that out for now

@andrewbranch andrewbranch added the Fix Available A PR has been opened for this issue label Jun 9, 2020
@jrieken
Copy link
Member

jrieken commented Sep 1, 2020

@mjbvz did we adopt this? I don't see a difference when selfhosting

@mjbvz
Copy link
Contributor Author

mjbvz commented Sep 1, 2020

@andrewbranch Looks like TS does not return a value for containerName. It's undefined in all the cases I've tested

@mjbvz mjbvz reopened this Sep 1, 2020
@andrewbranch
Copy link
Member

It turns out there’s a data conversion function on the server layer, which our normal test suite doesn’t touch, that returns an object with a specific set of keys, so the new property was left out of the serialization. PR incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Relates to the public API for TypeScript Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone
Projects
None yet
5 participants