R117883810 @redirected placement #805

patshaughnessy · 2024-01-18T23:05:10Z

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

Summary

Today DocC does not parse the @Redirected directive properly if it appears before the abstract in an article or documentation extension. If the directive appears before the abstract, the abstract's contents is set to the directive only and the actual abstract is instead considered to be part of the following discussion.

For example, compiling this article:

# Getting Started with Sloths

@Redirected(from: "some/path")

Create a sloth and assign personality traits and abilities.

## Overview

Sloths are complex creatures that require careful creation and a suitable habitat. After creating a sloth, you're responsible for feeding them, providing fulfilling activities, and giving them opportunities to exercise and rest.

...would produce this page:

Note the abstract appears under "Overview", and that there are two "Overview" sections.

This fix removes any @Redirected directives from markdown content while parsing the title, abstract and discussion. Now DocC no longer considers @Redirected directives to be part of the markdown content at all, similar to how @Comment, @Metadata and @Options work. DocC saves the redirects' old path strings behind the scenes.

A related fix in this PR adds @Redirected as a child directive of @Metadata. This gives the author more flexibility to place @Redirected commands anywhere, including inside of the page's metadata which is the most logical place for it to appear.

Dependencies

None.

Testing

Create a new article which contains redirects after the title, but before the abstract as shown above in the Getting Started With Sloths page.
Compile and check the rendered article shows the abstract properly:

Repeat the same test, but move the @Redirected directive inside the @Metadata directives as a child. For example:

# Getting Started with Sloths

@Metadata {
    @Redirected(from: "some/path")
}

Create a sloth and assign personality traits and abilities.

## Overview

Sloths are complex creatures that require careful creation and a suitable habitat. After creating a sloth, you're responsible for feeding them, providing fulfilling activities, and giving them opportunities to exercise and rest.

The page should render correctly again.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

[x ] Added tests
[x ] Ran the ./bin/test script and it succeeded
~~Updated documentation if necessary~~

while parsing titles, abstracts and discussions, allowing them to appear anywhere after the title.

patshaughnessy · 2024-01-18T23:40:50Z

@swift-ci please test

d-ronnqvist

The code looks good to me.

Since it's a user facing enhancements it would be nice to document that @Redirected is supported in @Metadata as of 5.11.

I don't remember if running swift run generate-symbol-graph is sufficient to list @Redirected is a supported directive inside of @Metadata, since both directives are AutomaticDirectiveConvertible. Regardless, we should probably mention in a note that it's only available in 5.11 and later (otherwise it will look like it's been supported since 5.5 (since both directives are marked as being introduced then)

d-ronnqvist · 2024-01-19T18:02:43Z

Sources/SwiftDocC/Model/DocumentationMarkup.swift

+        Metadata.directiveName,
+        Options.directiveName,
+        Redirect.directiveName,
+    ]


I think maybe that we should also add DeprecationSummary to this list (see rdar://79719308)—but that could fit better in a separate PR.

Sure sounds good. I can take that Radar.

d-ronnqvist · 2024-01-19T18:48:32Z

Tests/SwiftDocCTests/Model/DocumentationMarkupTests.swift

+        func checkSectionContent(expectedContent: String?, section: Section?) {
+            if let desc = section?.content.map({ $0.detachedFromParent.debugDescription() }).joined(separator: "\n") {
+                XCTAssertEqual(expectedContent, desc, "Found unexpected content: \n\(desc)")


When adding test helpers like this it's helpful to given them file and line arguments that default to #file and #line and then also pass those values to all the XCTAssert calls. This makes it so that if the test fails then the failure will be attributed to the location that called checkSectionContent instead of the location inside of that function. It barely matters if the test helper is local to the same file, but if it's a general test helper in a separate file it can make a big difference.

Suggested change

func checkSectionContent(expectedContent: String?, section: Section?) {

if let desc = section?.content.map({ $0.detachedFromParent.debugDescription() }).joined(separator: "\n") {

XCTAssertEqual(expectedContent, desc, "Found unexpected content: \n\(desc)")

func checkSectionContent(expectedContent: String?, section: Section?, file: StaticString = #file, line: UInt = #line) {

if let desc = section?.content.map({ $0.detachedFromParent.debugDescription() }).joined(separator: "\n") {

XCTAssertEqual(expectedContent, desc, "Found unexpected content: \n\(desc)", file: file, line: line)

Thanks great suggestion 👍 Updated.

d-ronnqvist · 2024-01-19T18:50:59Z

Sources/SwiftDocC/Semantics/Metadata/Metadata.swift

@@ -87,6 +90,7 @@ public final class Metadata: Semantic, AutomaticDirectiveConvertible {
        "supportedLanguages"    : \Metadata._supportedLanguages,
        "_pageColor"            : \Metadata.__pageColor,
        "titleHeading"          : \Metadata._titleHeading,
+        "redirects"              : \Metadata._redirects,


Apologies for the super nit but the misalignment bugged me

Suggested change

"redirects" : \Metadata._redirects,

"redirects" : \Metadata._redirects,

ha ha no problem - updated

d-ronnqvist · 2024-01-19T18:51:37Z

Sources/SwiftDocC/Model/DocumentationMarkup.swift

@@ -146,7 +154,7 @@ struct DocumentationMarkup {
                        // Found deprecation notice in the abstract.
                        deprecation = MarkupContainer(directive.children)
                        return
-                    } else if directive.name == Comment.directiveName || directive.name == Metadata.directiveName || directive.name == Options.directiveName {
+                    } else if Self.directivesRemovedFromContent.contains(directive.name) {


Nice. That's more easier to expand to cover more directives in the future. 👍

parameters.

patshaughnessy · 2024-01-24T22:13:40Z

@swift-ci please test

patshaughnessy · 2024-01-24T22:43:18Z

@swift-ci please test

patshaughnessy · 2024-01-24T22:44:52Z

Turns out that @Redirected wasn't documented at all. I've reenabled its docs, added curation so it appears under @Metadata, and also the note about version 5.11.

patshaughnessy · 2024-01-25T01:11:03Z

@swift-ci please test

sofiaromorales · 2024-01-25T18:25:14Z

Sources/SwiftDocC/Semantics/Redirect.swift

+///
+/// > Note: Starting with version 5.11, @Redirected is supported as a child directive of @Metadata. In
+/// previous versions, @Redirected must be used as a top level directive.
+///


Love this 💗

sofiaromorales · 2024-01-25T18:27:31Z

Sources/SwiftDocC/Semantics/Redirect.swift

-    static var hiddenFromDocumentation = true
+    static var hiddenFromDocumentation = false


@patshaughnessy I'm not following here, why is this needed?

We use the generate-symbol-graph command to document all of DocC's directives automatically. This command generates a symbol graph file for these, which is then used when we compile and publish documentation for DocC itself. This particular setting is used here to filter out some directives. (I'm not sure why we need to filter them at all, to be honest). Since hiddenFromDocumentation was set to true previously for @Redirected, I changed it to false to include docs for this again.

sofiaromorales

Thanks Pat!

patshaughnessy added 2 commits January 18, 2024 11:05

DocumentationMarkup removes @redirected directives from the content

60bc84d

while parsing titles, abstracts and discussions, allowing them to appear anywhere after the title.

Allow @redirected to be child directives of @metadata.

dca03c6

patshaughnessy requested review from d-ronnqvist and sofiaromorales January 18, 2024 23:07

Merge branch 'main' into r117883810-@redirected-placement

a76cdf9

d-ronnqvist reviewed Jan 19, 2024

View reviewed changes

patshaughnessy added 3 commits January 24, 2024 12:44

Merge branch 'main' into r117883810-@redirected-placement

207bab9

Document the @redirected directive.

3605e1f

Test helpers in DocumentationMarkupTests use file and line number

8c647e2

parameters.

patshaughnessy added 2 commits January 24, 2024 14:33

Modified files were updated in 2024

de9c56a

Fix code alignment in Metadata.swift

510282a

patshaughnessy requested a review from d-ronnqvist January 24, 2024 22:45

d-ronnqvist approved these changes Jan 25, 2024

View reviewed changes

sofiaromorales reviewed Jan 25, 2024

View reviewed changes

sofiaromorales approved these changes Jan 25, 2024

View reviewed changes

patshaughnessy merged commit 21c3e25 into swiftlang:main Jan 26, 2024

	"redirects" : \Metadata._redirects,
	"redirects" : \Metadata._redirects,

		static var hiddenFromDocumentation = true
		static var hiddenFromDocumentation = false

R117883810 @redirected placement #805

R117883810 @redirected placement #805

Uh oh!

Conversation

patshaughnessy commented Jan 18, 2024

Summary

Dependencies

Testing

Checklist

Uh oh!

patshaughnessy commented Jan 18, 2024

Uh oh!

d-ronnqvist left a comment

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!

patshaughnessy commented Jan 24, 2024

Uh oh!

patshaughnessy commented Jan 24, 2024

Uh oh!

patshaughnessy commented Jan 24, 2024

Uh oh!

patshaughnessy commented Jan 25, 2024

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!

sofiaromorales left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!