Skip to content
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

Filter out language-specific topic sections when creating See Also sections #1082

Merged

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Nov 1, 2024

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

Summary

This fixes a bug where language specific authored topic sections was considered when creating See Also sections for all language representations.

There's still a possible issue where a symbol with different canonical containers in different languages would pick the wrong container and not find the most appropriate topic section, but that requires some utilities from either #1081 or #1075 to fix, so I'm deferring that.

Dependencies

None

Testing

  • Recreate the setup from this new test with a few (at least 3) symbols that exist in more than one language and two language specific topic sections that each curate different subsets of those symbols.

  • Then preview the documentation for one of the symbols that is curated into both language specific topic sections.

    • On the Swift version of the page the See Also section at the bottom should display the title and links from the Swift-only topic section
    • On the Objective-C version of the page the See Also section at the bottom should display the title and links from the Objective-C-only topic section

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 Not applicable

Copy link
Contributor

@patshaughnessy patshaughnessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 I was able to observe the broken behavior on the main branch, and the fix in action on this branch.

Left a nerd snipe code suggestion :)

@@ -163,6 +163,7 @@ public struct AutomaticCuration {
return nil
}

// FIXME: The shortest path to the reference may not be applicable to the given variants traits.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we plan on addressing this soon? Should we open a GitHub issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I started working on this issue but it requires some replacement API that I introduced in a related PR so I put it on hold until those are merged. I opened this issue that refers to this code. I'll add a link in the code comment as well

reference != node.reference
&&
// Filter out nodes that aren't available in any of the given traits.
context.sourceLanguages(for: reference).contains(where: { variantLanguageIDs.contains($0.id) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just refactoring what we had earlier? Would it be even simpler to intersect the arrays somehow, vs. a nested loop? Not really a concern since the number of languages is usually 1 or 2. Would just be more readable.

Maybe something like this:

let variantLanguages = Set(variantsTraits.compactMap{ $0.interfaceLanguage }.map{ SourceLanguage(id: $0)})

etc...


// Filter out nodes that aren't available in any of the given traits.
!context.sourceLanguages(for: reference).intersection(variantLanguages).isEmpty

Although you use variantLanguageIDs above in isRelevant so we'd need some more refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed that the two separate filters could be expressed as once combined condition and since isRelevant also does a language check I wanted both places to do the same check (and felt that it was nice to get change one of the contains(where:) to a simpler contains(_:).

Because the languages is going to be incredibly small (either 1 or 2) any performance difference is not going to be noticeable so I can update it to use Set<SourceLanguage> in both places for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to use a set of languages in 6939109.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 3b39d07 into swiftlang:main Nov 8, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the language-specific-automatic-see-also branch November 8, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants