Filter topics languages in articles #103

franklinsch · 2022-03-11T18:18:22Z

Bug/issue #, if applicable: rdar://90159299

Summary

As a follow-up to #96, fixes an issue where variants of multi-language articles display APIs that are not available in the language variant. This fix makes DocC only display the APIs available in the language of the currently-selected language. For example, this fix makes DocC omit a Swift-only API from the Objective-C variant of an article that curates it.

I've broken up the changes in three commits:

The first commit adds map APIs to the VariantCollection family of types, to make it easier to update values in variant collections. This functionality is required for the next commit.
The actual fix which encodes topics sections on a per-variant basis, and fixes an issue where non-default variants of empty topic sections are encoded as [] instead of not at all.
Resolves the same issue, but for manually curated See Also sections.

Note: I noticed that automatically curated See Also sections for both articles and symbols don't get the filtering treatment. I will fix this in a follow-up PR.

Dependencies

None.

Testing

When building documentation for an article that curates symbols, verify that the render JSON topics and sections for each language variant only contain APIs that are available in that language. Also, verify the same behavior for automatically curated See Also sections.

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

franklinsch · 2022-03-11T18:19:39Z

@swift-ci please test

ethan-kusters · 2022-03-14T16:49:25Z

@swift-ci please test

ethan-kusters · 2022-03-14T16:51:48Z

Sources/SwiftDocC/Model/Rendering/Variants/VariantCollection+Variant.swift

+        }
+
+        /// Returns a new variant collection containing the traits of this variant collection with the values transformed by the given closure.
+        public func mapPatch<TransformedValue>(


I think this might be easier to understand at the call site without much added verbosity with just patch.map instead of the helper function mapPatch. What do you think?

Hmm, the reason I called it mapPatch was to align with Dictionary's mapValues. It would be confusing to have mapTraits and map, if we ever want to support mapping of the traits.

Ah I more meant- do we need this function at all?

Couldn't I just do:

<my-variant-collection>.patch.map { ... }

Instead of

<my-variant-collection>.patchMap { ... }

?

Ah, gotcha. So the map function in this case would return [VariantPatchOperation<TransformedValue>] and the client would still need to create a VariantCollection<TransformedValue> because they wouldn't be able to assign the result of the map to VariantCollection<Value> (different generic arguments). So I think keeping that complexity in this function makes sense

Ohhh. Tricky. Yup- that makes sense. Thanks!

ethan-kusters · 2022-03-14T16:56:56Z

Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift

+            var sections = [TaskGroupRenderSection]()
+
+            if let topics = article.topics, !topics.taskGroups.isEmpty {
+                // Don't set an eyebrow as collections and groups don't have one; append the authored Topics section


I'm seeing this comment that we shouldn't add eyebrow text for collection pages, but then I don't see the corresponding else branch where we do add eyebrow text for articles that don't have task groups. Am I missing something here?

Ah okay. Now I'm seeing that below outside of the topic sections variants bit.

if node.topicSections.isEmpty { // Set an eyebrow for articles node.metadata.roleHeading = "Article" }

Maybe the comment just doesn't apply any more and we should remove it? The ordering of when we set the eyebrow text versus set the topic sections shouldn't matter.

Final thought- we should probably make API collection pages only available in Objective-C or Swift if it curates languages of one or the other language.

Otherwise would need to call it an article in one variant and a collection group in the other which doesn't make sense.

Hmm, that's an interesting edge case. I'm not convinced that API collection pages should only be available in the language in which their topics are curated, because the API collection page could contain prose content that's interesting regardless of the language. It's easy to create an API collection accidentally (i.e., by just adding topics to an Article). Maybe we can move this conversation to the forums. Separate from that, this would be technically challenging because we'd need to parse the contents of the page before assigning its available languages.

Yeah that's fair. Maybe the right default behavior is that we should turn it into an Article in one language and an API collection page in the other.

Then as a future enhancement we should add a metadata directive that limits a given page to one language or the other.

Then as a future enhancement we should add a metadata directive that limits a given page to one language or the other.

I like that idea. A directive that forces the languages in which an article (and framework, maybe?) is available. Something to discuss in more detail in the forums.

I filed https://bugs.swift.org/browse/SR-15998 to track this.

ethan-kusters · 2022-03-14T17:00:31Z

Sources/SwiftDocC/Model/Rendering/Variants/VariantCollection+Coding.swift

+        variantCollection.mapValues { value in
+            // Encode `nil` if the value is empty, so that when the patch is applied, it effectively
+            // removes the default value.
+            value.isEmpty ? nil : value


Nice 👍🏻

Will this apply to every time we encode a collection to a variant? (Are there other code paths that achieve this similar thing?)

I ran into bugs where I would end up with an empty topic section instead of a removed one a few times so making this happen at the encoding level makes a ton of sense to me.

It will apply every time we call encodeVariantCollectionIfNotEmpty in the encoder.

I ran into bugs where I would end up with an empty topic section instead of a removed one a few times so making this happen at the encoding level makes a ton of sense to me.

Yes, this is exactly what this code is fixing :)

ethan-kusters · 2022-03-14T17:10:50Z

Tests/SwiftDocCTests/Model/SemaToRenderNodeMultiLanguageTests.swift

            line: line
        )

+        if let expectedSeeAlsoSectionIdentifiers = expectedSeeAlsoSectionIdentifiers {


Is it valid for this to be nil? Should we be XCTUnwraping instead?

No, this is valid. The optionality here is for callers of this test function to control whether they want to check for see also section identifiers (most of the tests don't need to).

Oh I see that now- thanks!

franklinsch · 2022-03-14T18:24:29Z

@swift-ci please test

Adds map APIs to variant collection types to facilitate transforming the value contained in variant collections.

For multi-language articles, only displays the APIs available in the language of the currently-selected language. For example, this fix makes DocC omit a Swift-only API from the Objective-C variant of an article that curates it. rdar://90159299

rdar://90159299

franklinsch · 2022-03-15T10:41:57Z

@swift-ci please test

franklinsch requested a review from ethan-kusters March 11, 2022 18:18

franklinsch requested review from QuietMisdreavus, d-ronnqvist and daniel-grumberg March 11, 2022 18:26

franklinsch force-pushed the filter-topics-languages-articles branch from 7f5ad7d to 229bf26 Compare March 11, 2022 18:30

franklinsch changed the title ~~Filter topics languages articles~~ Filter topics languages in articles Mar 14, 2022

franklinsch mentioned this pull request Mar 14, 2022

Filter topics languages for automatically-curated See Alsos #104

Merged

3 tasks

ethan-kusters reviewed Mar 14, 2022

View reviewed changes

franklinsch force-pushed the filter-topics-languages-articles branch from 229bf26 to 24a5756 Compare March 14, 2022 18:24

ethan-kusters approved these changes Mar 14, 2022

View reviewed changes

franklinsch added 3 commits March 15, 2022 10:41

Add map APIs to variant collection types

523dd35

Adds map APIs to variant collection types to facilitate transforming the value contained in variant collections.

Fix language availability for articles See Alsos

eed8c75

rdar://90159299

franklinsch force-pushed the filter-topics-languages-articles branch from 24a5756 to eed8c75 Compare March 15, 2022 10:41

franklinsch merged commit bef86be into swiftlang:main Mar 15, 2022

franklinsch deleted the filter-topics-languages-articles branch March 15, 2022 10:49

Filter topics languages in articles #103

Filter topics languages in articles #103

Uh oh!

Conversation

franklinsch commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dependencies

Testing

Checklist

Uh oh!

franklinsch commented Mar 11, 2022

Uh oh!

ethan-kusters commented Mar 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethan-kusters Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franklinsch commented Mar 14, 2022

Uh oh!

franklinsch commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

franklinsch commented Mar 11, 2022 •

edited

Loading

ethan-kusters Mar 14, 2022 •

edited

Loading