-
Notifications
You must be signed in to change notification settings - Fork 166
Filter topics languages for automatically-curated See Alsos #104
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -115,7 +115,14 @@ public struct AutomaticCuration { | |||||||
| /// - bundle: A documentation bundle. | ||||||||
| /// - Returns: A group title and the group's references or links. | ||||||||
| /// `nil` if the method can't find any relevant links to automatically generate a See Also content. | ||||||||
| static func seeAlso(for node: DocumentationNode, context: DocumentationContext, bundle: DocumentationBundle, renderContext: RenderContext?, renderer: DocumentationContentRenderer) throws -> TaskGroup? { | ||||||||
| static func seeAlso( | ||||||||
| for node: DocumentationNode, | ||||||||
| withTrait variantsTrait: DocumentationDataVariantsTrait, | ||||||||
| context: DocumentationContext, | ||||||||
| bundle: DocumentationBundle, | ||||||||
| renderContext: RenderContext?, | ||||||||
| renderer: DocumentationContentRenderer | ||||||||
| ) throws -> TaskGroup? { | ||||||||
| // First try getting the canonical path from a render context, default to the documentation context | ||||||||
| guard let canonicalPath = renderContext?.store.content(for: node.reference)?.canonicalPath ?? context.pathsTo(node.reference).first, | ||||||||
| !canonicalPath.isEmpty else { | ||||||||
|
|
@@ -125,6 +132,19 @@ public struct AutomaticCuration { | |||||||
|
|
||||||||
| let parentReference = canonicalPath.last! | ||||||||
|
|
||||||||
| func filterReferences(_ references: [ResolvedTopicReference]) throws -> [ResolvedTopicReference] { | ||||||||
| try references | ||||||||
| // Don't include the current node. | ||||||||
| .filter { $0 != node.reference } | ||||||||
|
|
||||||||
| // Filter out nodes that aren't available in the given trait. | ||||||||
| .filter { reference in | ||||||||
| try context.entity(with: reference) | ||||||||
|
Contributor
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. Should we be adopting this logic in the
Contributor
Author
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. We could, but we need to get the appropriate language-specific kind of the symbol anyway in that case. If we ported this logic to
Contributor
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.
What if we have the same kind in multiple languages?
Contributor
Author
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. The symbol would still have multiple kind entries, with the same value. |
||||||||
| .availableVariantTraits | ||||||||
| .contains(variantsTrait) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // Look up the render context first | ||||||||
| if let taskGroups = renderContext?.store.content(for: parentReference)?.taskGroups, | ||||||||
| let linkingGroup = taskGroups | ||||||||
|
|
@@ -134,7 +154,7 @@ public struct AutomaticCuration { | |||||||
| { | ||||||||
| // Group match in render context, verify if there are any other references besides the current one. | ||||||||
| guard linkingGroup.references.count > 1 else { return nil } | ||||||||
|
Contributor
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. Nit since this is a little out-of-scope, but the existing logic seems a little hacky to me and would break if we ever stop including the current reference in the linking group.
Suggested change
Contributor
Author
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. The automatic curation logic here finds a task group that contains this reference, removing this reference from the task group. The
Contributor
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. Right I see that- I'm just suggesting we do the filtering ahead of time so we can gate on the already filtered group being empty or not. let references = try filterReferences(linkingGroup.references)
guard !references.isEmpty else { return nil }
return (title: linkingGroup.title, references: references) |
||||||||
| return (title: linkingGroup.title, references: linkingGroup.references.filter { $0 != node.reference }) | ||||||||
| return (title: linkingGroup.title, references: try filterReferences(linkingGroup.references)) | ||||||||
| } | ||||||||
|
|
||||||||
| // Get the parent's task groups | ||||||||
|
|
@@ -152,7 +172,7 @@ public struct AutomaticCuration { | |||||||
| return nil | ||||||||
| } | ||||||||
|
|
||||||||
| return (title: group.title, references: group.references.filter { $0 != node.reference }) | ||||||||
| return (title: group.title, references: try filterReferences(group.references)) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
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.
Nit: Is there a more specific name we can use to make this more readable at the call-site?
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.
It also filters out the current reference from the task group, though. We could also consider making this function only handle the trait-based filtering, but the logic to filter out the current reference would then be duplicated.