-
Notifications
You must be signed in to change notification settings - Fork 158
Populate symbol kind in external render nodes #1251
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
Populate symbol kind in external render nodes #1251
Conversation
@swift-ci please test |
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 unsure about the usage of renderIdentifier
.
/// - documentationKind: The kind of external content that's being referenced. | ||
@_spi(ExternalLinks) | ||
public init(topicRenderReference: TopicRenderReference, renderReferenceDependencies: RenderReferenceDependencies, sourceLanguages: Set<SourceLanguage>) { | ||
public init(topicRenderReference: TopicRenderReference, renderReferenceDependencies: RenderReferenceDependencies, sourceLanguages: Set<SourceLanguage>, documentationKind: DocumentationNode.Kind) { |
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.
This is a public API, shouldn't we deprecate it and create a new initializer?
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 didn't do this because it's experimental API and comes with a warning:
swift-docc/Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolver.swift
Lines 44 to 45 in 65aaf92
@_spi(ExternalLinks) // This isn't stable API yet. | |
public struct ExternalEntity { |
WDYT? Happy to do as you suggested.
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've updated this init and now the new parameter is optional
externalID: renderNode.externalIdentifier.identifier, | ||
role: renderNode.role, | ||
symbolKind: renderNode.symbolKind?.identifier, | ||
symbolKind: renderNode.symbolKind?.renderingIdentifier, |
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.
From
var renderingIdentifier: String { |
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.
renderingIdentifier
is what we use for deriving the navigator symbol kind for local items -- for consistency I think we should use the same.
swift-docc/Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift
Lines 1299 to 1301 in 65aaf92
node.metadata.symbolKindVariants = VariantCollection<String?>(from: symbol.kindVariants) { _, kindVariants in | |
kindVariants.identifier.renderingIdentifier | |
} ?? .init(defaultValue: nil) |
/// The kind of external content that's being referenced. | ||
/// | ||
/// For example, the navigator requires specific knowledge about what type of external symbol is being linked to. | ||
var documentationKind: DocumentationNode.Kind |
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.
Is it necessary to store a DocumentationNode.Kind
if we only need to access the symbol identifier that's derived from it?
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.
Not necessary to store the whole documentation kind, as we need only the symbol kind. Updated to only store the symbol kind in e7bada9
var symbolKind: SymbolGraph.Symbol.KindIdentifier? { | ||
// Symbol kind information is not available for external entities | ||
return nil | ||
DocumentationNode.symbolKind(for: externalEntity.documentationKind) |
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.
Is this value different from externalEntity.topicRenderReference.role.map { init(rawValue: $0) }
?
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.
Yes, externalEntity.topicRenderReference.role
is derived using this function, which for symbols only returns .symbol
, and not which type of symbol it is.
The symbol kinds we're looking for here are more specific i.e. class
, func
.
There is a mapping from `SymbolGraph.Symbol.KindIdentifier` to `DocumentationNode.Kind`, but not the other way around; this commit adds one. For documentation kinds which both map to the same symbol kind, this has been expressed as: - both documentation kinds are converted to the same symbol kind in `symbolKind(for:)`. - a specific documentation kind is chosen as the default mapping in `kind(forKind:)` For example, for `DocumentationNode.Kind. typeConstant` [1]: - both `symbolKind(for: .typeConstant)` and `symbolKind(for: .typeProperty)` return `.typeProperty` - `kind(forKind: .typeProperty)` returns `.typeProperty` This function will be used to map and external entity's documentation kind to a symbol kind, so that that information can be populated as part of the navigator. ## Verification: This has been verified by testing the round trip conversion of all symbol kinds, and all documentation kinds which are symbols. [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Model/Kind.swift#L151
Adds a new property to `LinkResolver.ExternalEntity` which stores the symbol kind of the external entity. This is derived at the level of `OutOfProcessReferenceResolver.ResolvedInformation`, which has access to the documentation kind [1] and already uses it to derive the render node kind and role, but then discards it. This commit adds logic to `OutOfProcessReferenceResolver.ResolvedInformation` to derive the symbol kind from the documentation kind, then capture that in `LinkResolver.ExternalEntity. We need access to the documentation kind in order to resolve the symbol kind as part of computing the navigator title. [2] [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Infrastructure/External%20Data/OutOfProcessReferenceResolver.swift#L564-L565 [2]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/Navigator/RenderNode%2BNavigatorIndex.swift#L163
Propagates the symbol kind value from `LinkResolver.ExternalEntity` to the `ExternalRenderNode` used for generating the navigation index for external render nodes. This completes adding symbol kind support for external entities in the navigator. Also updates how the symbol kind is propagated to the navigator metadata, by using `.renderingIdentifier` over `.identifier`, to match the behaviour of local nodes [1]. Fixes rdar://156487928. [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift#L1300
8173968
to
ca3986e
Compare
@swift-ci please test |
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.
🔥
Bug/issue #, if applicable: rdar://156487928
Summary
Fixes the navigator page type for external render nodes such that it takes into account the kind of symbol of the node.
In order to properly determine the navigator page type for a symbol, we need to know its symbol kind:
swift-docc/Sources/SwiftDocC/Indexing/Navigator/RenderNode+NavigatorIndex.swift
Lines 148 to 168 in 65aaf92
However, the symbol kind was not available for external nodes and all items were showing as their generic role, e.g. "article" or "symbol".
This PR adds logic to derive an external render node's symbol kind:
OutOfProcessReferenceResolver.ResolvedInformation
's documentation kind toLinkResolver.ExternalEntity
, the type which is used to deal with external entities.Navigator comparison (using swift-docc-render):
The navigator icons on the right match the symbol kinds of the external pages.
The navigator
index.json
shows the correct page type as well:Dependencies
N/A.
Testing
Previewed a custom bundle locally which contained different types of external links (module, class, function and article).
index.json
looked as expectedSteps:
DOCC_LINK_RESOLVER_EXECUTABLE=Example.docc/bin/test-data-external-resolver swift run docc preview Example.docc
Example.docc/.docc-build/index/index.json
looks as expected.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded