Skip to content

Support writing links in synthesized technology root pages #980

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

Merged

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Jul 10, 2024

Bug/issue #, if applicable: rdar://129282701 rdar://127874910

Summary

This primarily fixes a bug where links were DocC didn't resolve any links in the content of a "synthesized" root page.

It also fixes an unrelated bug where an article that got promoted to a root page lost its existing metadata configuration.

Dependencies

None.

Testing

  • Create a documentation catalog with a single markup file without a @TechnologyRoot directive but with some links that won't resolve. For example:

    # Something
    
    The link below will fail to resolve:
    
    - ``NotFoundSymbol``
  • Build documentation for this article-only catalog.

    • There should be a warning about the link that doesn't resolve
  • Add a @PageColor metadata configuration to the page:

      # Something
    
    + @Metadata {
    +   @PageColor(red)
    + }
    
      The link below will fail to resolve:
    
      - ``NotFoundSymbol``
  • Build and preview documentation for this article-only catalog.

    • There should still be a warning about the link that doesn't resolve.
    • The page should have the custom page color.
  • Add another article and name it the same as the base name of the documentation catalog. For example:

    # Something else
    
    Another article
  • In the first article, go back and add a link to the second article. For example:

      # Something
    
      @Metadata {
        @PageColor(red)
      }
    
      The link below will fail to resolve:
    
      - ``NotFoundSymbol``
    + - <doc:CatalogName>
  • Build and preview documentation for this article-only catalog.

    • The second article should now the be root, because it's name matches the catalog's name.
      • This root page doesn't have a custom page color.
    • There should still be a warning about the symbol link that doesn't resolve.
    • The link from the first article to the second should resolve and work when clicked on.

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
  • [x] Updated documentation if necessary Not applicable

Also, avoid overriding existing metadata when promoting articles to root pages.

rdar://129282701
@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

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.

LGTM 👍

let reference = ResolvedTopicReference(
bundleIdentifier: bundle.identifier,
path: NodeURLGenerator.Path.documentation(path: title).stringValue,
sourceLanguages: [DocumentationContext.defaultLanguage(in: [])]
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should give in: a default value of [] or nil so this reads a bit better:

sourceLanguages: [DocumentationContext.defaultLanguage]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is only a private method I would prefer to not add a default value so that future callers have to think about what languages they pass.

If there was a default value there's a slight risk that a future caller that has available language information uses the default value and gets the wrong result back which could lead to a subtle bug. Having to explicitly pass nil makes this a tiny bit more explicit than passing no value (possibly due to autocompletion).

I added an inline comment that provides additional context to why nil is passed here.

let metadataMarkup: BlockDirective
if let markup = articleResult.value.metadata?.originalMarkup as? BlockDirective {
metadataMarkup = markup.withUncheckedChildren(
markup.children + [BlockDirective(name: "TechnologyRoot", children: [])]
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we know the existing article doesn't already have a @TechnologyRoot directive? I suppose because this entire code path under synthesizeArticleOnlyRootPage wouldn't occur if it did. But we might want to add an assertion here that the existing article doesn't already have @TechnologyRoot so we can be sure we aren't adding a duplicate directive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. The context wouldn't call synthesizeArticleOnlyRootPage (which is a private method) if it already had an explicitly authored root page or module page.

Still, I added a debug assertion here to help catch potentially mistakes in the future.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

@d-ronnqvist d-ronnqvist merged commit 9a5be7e into swiftlang:main Jul 17, 2024
2 checks passed
@d-ronnqvist d-ronnqvist deleted the links-in-synthesized-root-pages branch July 17, 2024 09:54
d-ronnqvist added a commit to d-ronnqvist/swift-docc that referenced this pull request Jul 17, 2024
…#980)

* Use a correct root reference for synthesized root pages.

Also, avoid overriding existing metadata when promoting articles to root pages.

rdar://129282701
rdar://127874910

* Add debug assert about not synthesizing a root page when one exists
Add comment about article-only default language
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