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

Refine logic for when curation removes a symbol from automatic curation #1075

Merged

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Oct 24, 2024

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

Summary

This refines the logic for when authored curation would remove a symbol from automatic curation.

Before this, any authored curation would remove a symbol or article from automatic curation.

With these changes, a symbol isn't removed from automatic curation if it's curated outside of its canonical container.

This new code comment explains the new logic well:

For symbols we only stop automatic curation if they are curated within their canonical container's sub-hierarchy or if a top-level symbol is curated under another top-level symbol (more on that below).

For example, curating a member under an API collection within the container removes the member from automatic curation:

 ┆
 ├─SomeClass
 │ └─API Collection
 │   └─SomeClass/someMethod()  ◀︎━━ won't auto curate

However, curating a member outside under another container doesn't remove it from automatic curation:

 ┆
 ├─Other Container
 │ └─SomeClass/someMethod()  ◀︎━━  will still auto curate under `SomeClass`
 ├─SomeClass

The same applies if the authored curation location is a another member of the canonical container:

 ┆
 ├─SomeClass
 │ └─SomeClass/SomeInnerClass
 │   └─SomeClass/someMethod()  ◀︎━━ will still auto curate under `SomeClass`

Top-level symbols curated under other top-level is an exception to this rule.

 ┆
 ├─SomeClass
 │ └─OtherTopLevelClass  ◀︎━━ won't auto curate because it's top-level.

The reason for this exception is to allow developers to group top-level types under one-another without requiring an API collection. For example, in DocC one could curate DiagnosticConsumer, DiagnosticFormattingOptions, and Diagnostic under DiagnosticEngine, treating the DiagnosticEngine as the top-level topic for all diagnostic related types.

These added tests also verify when specific types curation do or don't remove a symbol from automatic curation.

Dependencies

None.

Testing

In a project with some symbols:

  • Curate a top-level symbol under its module.
    • This will stop automatically curating that symbol.
  • Curate a top-level symbol under an API collection under the symbol's module.
    • This will stop automatically curating that symbol.
  • Curate a top-level symbol under another top-level symbol.
    • This will stop automatically curating that symbol.
  • Curate a top-level symbol under a member of another symbol.
    • This won't stop automatically curating the top-level symbol.
  • Curate a member of some top-level symbol under a different top-level symbol.
  • This won't stop automatically curating the member symbol.
  • Curate an article under a member of a container symbols.
    • This will stop automatically curating that article.

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

@d-ronnqvist d-ronnqvist force-pushed the refine-when-symbols-automatically-curate branch from ad776a2 to 399de98 Compare October 30, 2024 17:13
@d-ronnqvist d-ronnqvist force-pushed the refine-when-symbols-automatically-curate branch from 3e1458d to 39bd4c7 Compare October 31, 2024 15:29
@d-ronnqvist d-ronnqvist marked this pull request as ready for review October 31, 2024 15:29
# Conflicts:
#	Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver+Breadcrumbs.swift
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist requested a review from anferbui November 8, 2024 17:20
Copy link
Contributor

@anferbui anferbui 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 to me, left some small comments.

@@ -2409,7 +2409,7 @@ public class DocumentationContext {
///
/// - Parameter automaticallyCurated: A list of automatic curation records.
func removeUnneededAutomaticCuration(_ automaticallyCurated: [AutoCuratedSymbolRecord]) {
// It might look like it would be correct to check `topicGraph.nodes[symbol]?.isManuallyCurated` here,
// It might look like it would be correct to check `topicGraph.nodes[symbol]?.isManuallyCuratedInContainer` here,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be shouldAutoCurateInCanonicalLocation?

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, thank you. I renamed that property more than once for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f4265d3

Comment on lines 213 to 216
> Note:
> If you organize a member symbol into a topic group outside of its canonical container symbol,
> DocC will still include the member symbol in the default topic group.
> This ensures that the reader can always find the member symbol somewhere in the sub-hierarchy of its canonical container symbol.
Copy link
Contributor

Choose a reason for hiding this comment

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

An example here might be helpful, as I'm not sure how recognisable the term "canonical container symbol" will be for those reading this documentation.

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 rephrased this in f788950 to "the type that defines the member". That sentence gets a bit confusing with symbols that have different locations in different language representations but I think overall fewer people will be confused by it.

d-ronnqvist and others added 4 commits November 12, 2024 17:24
Co-authored-by: Andrea Fernandez Buitrago <15234535+anferbui@users.noreply.github.com>
- Update reference to topic graph node property in code comment
- Rephrase public documentation about curating a symbol outside its canonical container
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 50c9904 into swiftlang:main Nov 13, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the refine-when-symbols-automatically-curate branch November 13, 2024 13:17
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