-
Notifications
You must be signed in to change notification settings - Fork 147
Populate abbreviated declaration fragments in external render nodes #1255
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
Open
anferbui
wants to merge
4
commits into
swiftlang:main
Choose a base branch
from
anferbui:declaration-fragments-in-external-render-nodes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
832f1eb
Add the abbreviated declaration fragments to LinkDestinationSummary
anferbui 86b1ff8
Use the subheading fragments to init the external entity
anferbui 79c9844
Populate declaration fragments in navigator metadata for external links
anferbui 2785db2
fixup! Use the subheading fragments to init the external entity
anferbui File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,7 +117,8 @@ struct NavigatorExternalRenderNode: NavigatorIndexableRenderNodeRepresentation { | |
externalID: renderNode.externalIdentifier.identifier, | ||
role: renderNode.role, | ||
symbolKind: renderNode.symbolKind?.identifier, | ||
images: renderNode.images | ||
images: renderNode.images, | ||
fragments: renderNode.fragmentsVariants.value(for: traits) | ||
) | ||
} | ||
} | ||
|
@@ -130,19 +131,16 @@ struct ExternalRenderNodeMetadataRepresentation: NavigatorIndexableRenderMetadat | |
var role: String? | ||
var symbolKind: String? | ||
var images: [TopicImage] | ||
var fragments: [DeclarationRenderSection.Token]? | ||
|
||
// Values that we have insufficient information to derive. | ||
// These are needed to conform to the navigator indexable metadata protocol. | ||
// | ||
// The fragments that we get as part of the external link are the full declaration fragments. | ||
// These are too verbose for the navigator, so instead of using them, we rely on the title, navigator title and symbol kind instead. | ||
// | ||
Comment on lines
-137
to
-139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment and behavior remains true. IIRC even the shorter sub heading fragments include argument types but we typically don't display that in the navigator. |
||
// The role heading is used to identify Property Lists. | ||
// The value being missing is used for computing the final navigator title. | ||
// | ||
// The platforms are used for generating the availability index, | ||
// but doesn't affect how the node is rendered in the sidebar. | ||
var fragments: [DeclarationRenderSection.Token]? = nil | ||
var roleHeading: String? = nil | ||
var platforms: [AvailabilityRenderItem]? = nil | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If the subheading declaration fragments it's used instead of the fragments in the TopicRenderReference why do we want to keep declarationFragments here instead of only keeping the subheading one?
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.
If we don't need the full declaration fragments for anything then we should repurpose the
declarationFragments
property instead.I consider
OutOfProcessResolver.ResolvedInformation
to almost be a deprecated type at this point (see #802).It could also be good to check if implementing #802 would fix this issue. If so, it might be worth using this use-case as a driver to implement that and get rid of this code duplication that loses certain data richness.