Skip to content

Conversation

@franklinsch
Copy link
Contributor

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

Summary

Don't emit variants for article-only catalogs

For article-only catalogs, i.e., catalogs that use @TechnologyRoot, don't emit the variants array in the render node, so that renderers don't display the page's language, since it doesn't have a language.

A longer-term fix here would be to not consider articles in article-only catalogs to be "Swift", and implement logic in RenderNodeTranslator to not emit variants information for language-less pages (#240).

Note: The first commit just moves a test utility so that it can be used in a test introduced by the second commit.

Dependencies

None.

Testing

Build documentation for an article-only catalog, and verify that the "Language: Swift" label doesn't appear for any of its pages.

Checklist

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

@franklinsch franklinsch requested a review from ethan-kusters May 9, 2022 14:59
@franklinsch
Copy link
Contributor Author

@swift-ci please test

// Emit variants only if we're not compiling an article-only catalog to prevent renderers from
// advertising the page as "Swift", which is the language DocC assigns to pages in article only pages.
// (github.com/apple/swift-docc/issues/240).
if let topLevelModule = context.soleRootModuleReference,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about this scaling well in the future. Here's a few questions that came to mind- let me know what you think.

  • Should we consider just not emitting variants for documentation nodes that only contain a single variant? Is there a use case for the variant then?

  • Why are we only fixing this issue for TechnologyRoot catalogs? Should we fix this for any article that doesn't define a language?

  • In the future when we add support for articles with multi-language variants won't this break multi-language articles that are authored in TechnologyRoot catalogs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are great questions!

Should we consider just not emitting variants for documentation nodes that only contain a single variant? Is there a use case for the variant then?

This is still useful for renderers to know what language the page is available in for pages originating from frameworks, so that they can display e.g., "Language: Swift" or "Language: Objective-C" in the UI. From the render JSON perspective, the data is "correct", i.e., the page does have that variant. If we decide to not display the language information for single-variant pages, then I'd argue the change should be made in DocC Render, or at least controlled by a separate property in the JSON.

Why are we only fixing this issue for TechnologyRoot catalogs? Should we fix this for any article that doesn't define a language?

If associated with a module, articles inherit the module's languages, per #81.

In the future when we add support for articles with multi-language variants won't this break multi-language articles that are authored in TechnologyRoot catalogs?

If we add support for specifying what languages an article is available in, then yes, this logic will need to be updated and we'll likely want to implement a special undefined language per #240. But I think we should consider these changes as part of that later fix. I'm also planning on cherry-picking this onto 5.7, so I'd like to minimize risk, and introducing a new special undefined language would be a riskier change (e.g., we'd need to update the navigator index logic).

Copy link
Contributor

Choose a reason for hiding this comment

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

That all seems reasonable to me. Thank you!

@franklinsch
Copy link
Contributor Author

@swift-ci please test

@franklinsch franklinsch force-pushed the article-only-language-variants branch from 70b5672 to 6006687 Compare May 12, 2022 09:45
@franklinsch
Copy link
Contributor Author

@swift-ci please test

Refactor TestRenderNodeOutputConsumer into its own file so that it can
be used in other tests.
@franklinsch franklinsch force-pushed the article-only-language-variants branch from 6006687 to f6b9a48 Compare June 9, 2022 18:41
For article-only catalogs, i.e., catalogs that use @TechnologyRoot,
don't emit the variants array in the render node, so that renderers
don't display the page's language, since it doesn't have a language.

A longer-term fix here would be to not consider articles in article-only
catalogs to be "Swift", and implement logic in RenderNodeTranslator to
not emit variants information for language-less pages
(github.com/swiftlang/issues/240).

rdar://92758192
@franklinsch franklinsch force-pushed the article-only-language-variants branch from f6b9a48 to 49b9334 Compare June 9, 2022 18:47
@franklinsch
Copy link
Contributor Author

@swift-ci please test

@franklinsch franklinsch merged commit b3c12e9 into swiftlang:main Jun 9, 2022
@franklinsch franklinsch deleted the article-only-language-variants branch June 9, 2022 18:56
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.

2 participants