Create LinkDestinationSummary elements with multi-language variant content for symbols #60

d-ronnqvist · 2021-12-16T20:17:49Z

Bug/issue #, if applicable: SR-15574 rdar://86298116

Summary

This change creates LinkDestinationSummary elements with multi-language variant content for symbols. Before this change only the main summary element was created without its variant content.

Dependencies

n/a

Testing

Convert a multi-language documentation catalog and include the --emit-digest flag so that Swift-DocC will create the linkable-entities.json file.

(For testing purposes it may be convenient to also set the DOCC_JSON_PRETTYPRINT environment to YES to get output that's more easily inspectable.)

The content of the linkable-entities.json file should include the variant content of each interface language for each symbol "entity". Each variant should contain only the information that's different in the variant compared to the summarized element.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

…ntent for symbols rdar://86298116

d-ronnqvist · 2021-12-20T21:41:48Z

@swift-ci please test

d-ronnqvist · 2021-12-21T20:19:42Z

@swift-ci please test

d-ronnqvist · 2021-12-23T18:32:57Z

@swift-ci please test

ethan-kusters

Looks great! Just left a few questions.

ethan-kusters · 2022-01-06T19:33:08Z

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

+        // Precompute the summarized elements information so that variants can compare their information against it and remove redundant duplicate information.
+
+        // Multi-language symbols need to access the default content via the variant accessors (rdar://86580516)
+        let kind = DocumentationNode.kind(forKind: (symbol.kindVariants[summaryTrait] ?? symbol.kindVariants[.fallback] ?? symbol.kind).identifier)


I think these chained ?? checks might be redundant.

The DocumentationDataVariants subscript looks like this:

public subscript(trait: DocumentationDataVariantsTrait) -> Variant? { get { values[trait] ?? defaultVariantValue } set { if trait == .fallback { defaultVariantValue = newValue } else { values[trait] = newValue } } }

So the .fallback value is the defaultVariantValue and we always fallback to the defaultVariantValue if the given trait doesn't exist. So that should handle the ?? symbol.kindVariants[.fallback] part inherently.

And the base symbol.kind value is already a wrapper around the kindVariants so I don't think we should be explicitly falling back to it.

We could do an explicit force unwrap of kindVariants.first! which I think is what symbol.kind is doing.

public var kind: SymbolGraph.Symbol.Kind { kindVariants.firstValue! }

In general I think we should probably try to avoid using the non-variant types on Symbol since we should deprecate them soon and they're a little confusing.

Does that make sense or have I missed a case here?

Suggested change

let kind = DocumentationNode.kind(forKind: (symbol.kindVariants[summaryTrait] ?? symbol.kindVariants[.fallback] ?? symbol.kind).identifier)

let kind = DocumentationNode.kind(forKind: (symbol.kindVariants[summaryTrait] ?? symbol.kindVariants.first!).identifier)

I don't love how we're having to force unwrap kindVariants.first here, it seems like defaultVariantValue should automatically fall back to the first variant and be non-optional if we want to guarantee that a variant collection always has a single value. Maybe @franklinsch wants to chime in here.

(I think this comment applies to several places in this function but I think we can isolate the discussion here)

I think force-unwrapping first is correct. This value is never expected to be nil in this case, because the kind property always had a value before (non-optional)

If the behavior is the same, I would prefer to call symbol.kind (and symbol.title, etc.) so that this code behaves consistently with how those properties are computed (in case that ever changes). That also helps avoid mistakes with force-unwrapping first when the symbol convenience property is optional.

ethan-kusters · 2022-01-06T19:36:36Z

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

+
+            let abstractVariant: Abstract?? = symbol.abstractVariants[trait].map { renderSymbolAbstract($0) }
+
+            func nilIfSame<Value: Equatable>(main: Value, variant: Value?) -> Value? {


Nit:

Suggested change

func nilIfSame<Value: Equatable>(main: Value, variant: Value?) -> Value? {

func nilIfEqual<Value: Equatable>(main: Value, variant: Value?) -> Value? {

ethan-kusters · 2022-01-06T22:20:22Z

Tests/SwiftDocCTests/LinkTargets/LinkDestinationSummaryTests.swift

+            XCTAssertEqual(summary.availableLanguages.sorted(by: \.id), [.objectiveC, .swift])
+            XCTAssertEqual(summary.platforms, renderNode.metadata.platforms)
+            XCTAssertEqual(summary.usr, "c:objc(cs)Bar")
+            //


Stray open comment delimiter here

franklinsch · 2022-01-07T13:50:34Z

Sources/SwiftDocC/Model/Rendering/Variants/VariantPatchOperation.swift

@@ -34,3 +34,24 @@ public enum VariantPatchOperation<Value: Codable> {
        }
    }
 }
+
+extension VariantCollection.Variant where Value: RangeReplaceableCollection {
+    /// Applies the variant's patch operations o a given value and returns the patched value


Suggested change

/// Applies the variant's patch operations o a given value and returns the patched value

/// Applies the variant's patch operations to a given value and returns the patched value.

franklinsch · 2022-01-07T13:54:43Z

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

        }
-        return [LinkDestinationSummary(documentationNode: self, path: presentationURL.path, taskGroups: taskGroups, platforms: platforms, compiler: &compiler)] + landmarkSummaries
+        return [LinkDestinationSummary(documentationNode: self, path: presentationURL.path, taskGroups: taskGroups, variantTaskGroups: variantTaskGroups, platforms: platforms, compiler: &compiler)] + landmarkSummaries


Nit: variantTaskGroups -> taskGroupVariants to align with the other variant APIs.

franklinsch · 2022-01-07T13:59:11Z

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

        let taskGroups: [LinkDestinationSummary.TaskGroup]
        switch kind {
        case .tutorial, .tutorialArticle, .technology, .technologyOverview, .chapter, .volume, .onPageLandmark:
            taskGroups = [.init(title: nil, identifiers: context.children(of: reference).map { $0.reference.absoluteString })]
        default:
            taskGroups = renderNode.topicSections.map { group in .init(title: group.title, identifiers: group.identifiers) }
+            for variant in renderNode.topicSectionsVariants.variants {
+                variantTaskGroups[variant.traits] = variant.applyingPatchTo(renderNode.topicSections).map { group in .init(title: group.title, identifiers: group.identifiers) }


Instead of implementing applyingPatchTo, which partly overlaps with JSONPatchApplier's functionality, we can instead use the RenderNodeVariantOverridesApplier API that applies the patch onto the entire render node. However, this will require first encoding the render node to JSON, which is probably undesirable for performance.

Yes, I think that will have a performance impact. When linkable entities are emits, this code runs for every page and landmark that have topic section variants. In the worst case this could end up encoding all the pages.

franklinsch · 2022-01-07T14:03:58Z

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

+        // Precompute the summarized elements information so that variants can compare their information against it and remove redundant duplicate information.
+
+        // Multi-language symbols need to access the default content via the variant accessors (rdar://86580516)
+        let kind = DocumentationNode.kind(forKind: (symbol.kindVariants[summaryTrait] ?? symbol.kindVariants[.fallback] ?? symbol.kind).identifier)


I think force-unwrapping first is correct. This value is never expected to be nil in this case, because the kind property always had a value before (non-optional)

- Remove redundant retrieval of `.fallback` trait values - Rename inline helper function to `nilIfEqual` - Rename internal argument to `taskGroupVariants` for consistency - Fix typo in documentation comment

d-ronnqvist · 2022-01-10T17:21:04Z

@swift-ci please test

d-ronnqvist · 2022-01-12T02:48:40Z

@swift-ci please test

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift

d-ronnqvist · 2022-01-21T18:17:38Z

@swift-ci please test

ethan-kusters

Thanks David!

d-ronnqvist · 2022-01-22T01:34:33Z

@swift-ci please test

Create LinkDestinationSummary elements with multi-language variant co…

da1eb9a

…ntent for symbols rdar://86298116

d-ronnqvist requested a review from ethan-kusters December 16, 2021 20:17

Merge branch 'main' into summarize-symbol-variants

e0bae01

ethan-kusters reviewed Jan 6, 2022

View reviewed changes

franklinsch reviewed Jan 7, 2022

View reviewed changes

d-ronnqvist added 3 commits January 7, 2022 17:48

Merge branch 'main' into summarize-symbol-variants

4f2f716

Address code review feedback:

11dba04

- Remove redundant retrieval of `.fallback` trait values - Rename inline helper function to `nilIfEqual` - Rename internal argument to `taskGroupVariants` for consistency - Fix typo in documentation comment

Resolve unrelated warning about a tuple label mismatch

16686be

Specify additional type information to avoid ambiguity

f895ff4

ethan-kusters reviewed Jan 21, 2022

View reviewed changes

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift Show resolved Hide resolved

Sources/SwiftDocC/LinkTargets/LinkDestinationSummary.swift Outdated Show resolved Hide resolved

d-ronnqvist added 2 commits January 21, 2022 10:11

Use VariantValue alias to avoid double optional declaration

7e6a541

Merge branch 'main' into summarize-symbol-variants

39c4d57

ethan-kusters approved these changes Jan 21, 2022

View reviewed changes

Merge branch 'main' into summarize-symbol-variants

ebdf195

d-ronnqvist merged commit 8f0b118 into apple:main Jan 22, 2022

d-ronnqvist mentioned this pull request Dec 16, 2021

[SR-15574] Create LinkDestinationSummary content variants for all source languages #198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create LinkDestinationSummary elements with multi-language variant content for symbols #60

Create LinkDestinationSummary elements with multi-language variant content for symbols #60

d-ronnqvist commented Dec 16, 2021

d-ronnqvist commented Dec 20, 2021

d-ronnqvist commented Dec 21, 2021

d-ronnqvist commented Dec 23, 2021

ethan-kusters left a comment

ethan-kusters Jan 6, 2022

ethan-kusters Jan 6, 2022

franklinsch Jan 7, 2022

d-ronnqvist Jan 8, 2022

ethan-kusters Jan 6, 2022

ethan-kusters Jan 6, 2022

franklinsch Jan 7, 2022

franklinsch Jan 7, 2022

franklinsch Jan 7, 2022

d-ronnqvist Jan 8, 2022 •

edited

franklinsch Jan 7, 2022

d-ronnqvist commented Jan 10, 2022

d-ronnqvist commented Jan 12, 2022

d-ronnqvist commented Jan 21, 2022

ethan-kusters left a comment

d-ronnqvist commented Jan 22, 2022

	let kind = DocumentationNode.kind(forKind: (symbol.kindVariants[summaryTrait] ?? symbol.kindVariants[.fallback] ?? symbol.kind).identifier)
	let kind = DocumentationNode.kind(forKind: (symbol.kindVariants[summaryTrait] ?? symbol.kindVariants.first!).identifier)


		let abstractVariant: Abstract?? = symbol.abstractVariants[trait].map { renderSymbolAbstract($0) }

		func nilIfSame<Value: Equatable>(main: Value, variant: Value?) -> Value? {

	func nilIfSame<Value: Equatable>(main: Value, variant: Value?) -> Value? {
	func nilIfEqual<Value: Equatable>(main: Value, variant: Value?) -> Value? {

	/// Applies the variant's patch operations o a given value and returns the patched value
	/// Applies the variant's patch operations to a given value and returns the patched value.

Create LinkDestinationSummary elements with multi-language variant content for symbols #60

Create LinkDestinationSummary elements with multi-language variant content for symbols #60

Conversation

d-ronnqvist commented Dec 16, 2021

Summary

Dependencies

Testing

Checklist

d-ronnqvist commented Dec 20, 2021

d-ronnqvist commented Dec 21, 2021

d-ronnqvist commented Dec 23, 2021

ethan-kusters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ronnqvist Jan 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ronnqvist commented Jan 10, 2022

d-ronnqvist commented Jan 12, 2022

d-ronnqvist commented Jan 21, 2022

ethan-kusters left a comment

Choose a reason for hiding this comment

d-ronnqvist commented Jan 22, 2022

d-ronnqvist Jan 8, 2022 •

edited