Skip to content

Show container name as the detail of a call hierarchy item #1165

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

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Apr 4, 2024

The container name, showing the class a method is defined on, is more useful than the module name.

@ahoppen
Copy link
Member Author

ahoppen commented Apr 4, 2024

@swift-ci Please test

@@ -2472,14 +2472,28 @@ extension IndexSymbolKind {
}
}

fileprivate extension IndexSymbolKind {
/// Whether this symbol should be considered as a container in `SymbolOccurrence.containerName`
var isContainer: Bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this should have something to do with type in its name (though also includes module/namespace...) given it doesn't include functions. Same with containerName.

Also union should probably be included in the true case either way.

Copy link
Contributor

@hamishknight hamishknight Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be best to avoid using the word container too, to avoid confusion with the containedBy relation. Maybe something like hasChildren? Or canHaveChildren?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really this is just: Should this be considered for containerName and I think it’s actually easier to just have the switch inside the filter of containerName.

@@ -2143,7 +2143,7 @@ extension SourceKitLSPServer {
return CallHierarchyIncomingCall(
from: indexToLSPCallHierarchyItem(
symbol: related.symbol,
moduleName: definitionSymbolLocation?.moduleName,
containerName: definition?.containerName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should module be the fallback if there is no container?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think it’s better to leave the containerName empty for top-level functions. That way it’s also easier to tell top-level functions apart from member functions.

The container name, showing the class a method is defined on, is more useful than the module name.
@ahoppen ahoppen force-pushed the container-name-as-call-hierarchy-detail branch from 224900d to 30bc65a Compare April 5, 2024 17:20
@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2024

@swift-ci Please test Windows

@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2024

@swift-ci Please test macOS

@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 44b8a82 into swiftlang:main Apr 8, 2024
3 checks passed
@ahoppen ahoppen deleted the container-name-as-call-hierarchy-detail branch April 8, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants