Make `@TechnologyRoot` optional in article-only documentation #778

d-ronnqvist · 2023-12-21T17:37:48Z

Bug/issue #, if applicable:

Summary

This makes @TechnologyRoot optional in article-only documentation.

When building documentation for a catalog of only articles with no explicit root article, DocC will follow the following logic to find a root:

If there's only a single article in the catalog, that becomes the root.
Otherwise, if there's an article with the same file name as the catalog name, that's becomes the root.
Otherwise, synthesize a minimal root with the same name as the catalog.

If this looks like a decent approach then we should document this behavior somewhere before merging.

Dependencies

None.

Testing

One article

Create a new documentation catalog:
mkdir MyCatalog.docc
Add a minimal article without a technology root:
echo "# One article\n\nArticle abstract" > MyCatalog.docc/MyArticle.md
Preview documentation for the new catalog
swift run docc preview MyCatalog.docc
- DocC should preview the article at http://localhost:8080/documentation/myarticle
  Question: should this path be "documentation/mycatalog" instead?
Stop the preview server.

Article with same name as catalog

Add another article without a technology root to the catalog, with the same name as the catalog this time:
echo "# Another article\n\nArticle abstract" > MyCatalog.docc/MyCatalog.md
Preview documentation for the catalog:
swift run docc preview MyCatalog.docc
- DocC should preview the article at http://localhost:8080/documentation/mycatalog
- MyArticle should be automatically curated.
Stop the preview server.

Synthesized root page

Rename the second article so that it's no longer named the same as the catalog:
mv MyCatalog.docc/MyCatalog.md MyCatalog.docc/OtherArticle.md
Preview documentation for the catalog:
swift run docc preview MyCatalog.docc
- DocC should preview the article at http://localhost:8080/documentation/mycatalog
- A new empty root page should be synthesized.
- Both MyArticle and OtherArticle should be automatically curated.
Stop the preview server.

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

sofiaromorales · 2024-01-02T15:00:29Z

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

+        guard markup.childCount == 2,
+              let blockDirective = markup.child(at: 1) as? BlockDirective,
+              let metadata = Metadata(from: blockDirective, for: bundle, in: self)
+        else {
+            throw ContextError.unableToSynthesizeRootPage
+        }


@d-ronnqvist Since markup is a constant and this is a programmer error would this not be better handled with an assertion?

Removing the guard statement also lets us get rid of the throwing function, which I think it makes sense here because there's nothing that could fail during runtime in this function.

I'm a little bit hesitant to trusting the parsed content completely since it also contains the # \(title) which comes from the bundle display name and I don't know if there's any content that a developer could accidentally put in there that would break the markup and cause a hard to debug crash.

That said, the markup is simple enough that I can manually construct it and avoid the parsing all together. I did this in 43ff6d4

sofiaromorales · 2024-01-02T15:23:33Z

@d-ronnqvist Thanks for working on this. I think this is a good idea, since it makes the entry barrier to this type of documentation way lower.

Some comments:

Did you consider adding index.md as another replacement of the technology root?. This could be a fallback in case there's no markdown file with the same name as the doc catalog, but there's an article with the file name index.
If we are moving ahead towards a direction where @TechnologyRoot is optional it might be a good idea to add some kind of deprecation note to that directive in the docs, where the use of the same name of the catalog, or index, is preferred over the use of the directive.
I left a minor code comment, but overall this looks good to me. To merge it, tho, it's needed to post info about this in the swift forums first.

Regarding your question:

should this path be "documentation/mycatalog" instead?

I think that keeping it as currently works is best, since it is consistent with the behaviour of docc for other types of catalogs, where the URL last path component is the same as the top level file name.

d-ronnqvist · 2024-01-02T18:49:21Z

Did you consider adding index.md as another replacement of the technology root?. This could be a fallback in case there's no markdown file with the same name as the doc catalog, but there's an article with the file name index.

I didn't but that's always something we can add later.

If we are moving ahead towards a direction where @TechnologyRoot is optional it might be a good idea to add some kind of deprecation note to that directive in the docs, where the use of the same name of the catalog, or index, is preferred over the use of the directive.

We should find some way to document that @TechnologyRoot isn't always needed but I don't think we should deprecate. A developer may prefer to add @TechnologyRoot over renaming one of their articles.

I left a minor code comment, but overall this looks good to me. To merge it, tho, it's needed to post info about this in the swift forums first.

I can post something small in the forums about this to make people aware of the change but I don't think that it needs to block the PR. I view this more as a small enhancement than as a new feature.

sofiaromorales · 2024-01-03T13:45:15Z

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

@@ -1874,6 +1874,52 @@ public class DocumentationContext: DocumentationContextDataProviderDelegate {
        }
    }

+    private func synthesizeArticleOnlyRootPage(articles: inout [DocumentationContext.SemanticResult<Article>], bundle: DocumentationBundle) {


Suggested change

private func synthesizeArticleOnlyRootPage(articles: inout [DocumentationContext.SemanticResult<Article>], bundle: DocumentationBundle) {

/// Assigns or creates a top level page to the documentation.

///

/// This method assigns a root to the only article in a single-article catalog,

/// to the article with the same file name as the catalog folder name,

/// or synthesizes a root with the same name as the catalog.

///

/// - Parameters:

/// - articles: Non-root articles from the catalog.

/// - bundle: The bundle containing the articles.

private func synthesizeArticleOnlyRootPage(articles: inout [DocumentationContext.SemanticResult<Article>], bundle: DocumentationBundle) {

sofiaromorales · 2024-01-03T13:47:26Z

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

+            var onlyArticle = articles.removeFirst()
+            onlyArticle.value = Article(markup: onlyArticle.value.markup, metadata: metadata, redirects: onlyArticle.value.redirects, options: onlyArticle.value.options)
+            registerRootPages(from: [onlyArticle], in: bundle)
+        } else if let nameMatchIndex = articles.firstIndex(where: { $0.source.deletingPathExtension().lastPathComponent == title }) {


This is what takes to add the "index" as possible root, + the unit test ofc.

Suggested change

} else if let nameMatchIndex = articles.firstIndex(where: { $0.source.deletingPathExtension().lastPathComponent == title }) {

} else if let nameMatchIndex = articles.firstIndex(where: { $0.source.deletingPathExtension().lastPathComponent == title || $0.source.deletingPathExtension().lastPathComponent == "index" }) {

// This catalog has an article with the same name as the catalog itself, or an article named index, so we make that the root.

Yes, it's very easy to implement another file name as a possible root but I'm not convinced there's a strong use case for it. If people want their "index.md" file to be a root for their article-only documentation they can always add a @TechnologyRoot directive to it.

What—in my opinion—makes the file matching the catalog name different is that that if we didn't turn that into a root page it would result in a "/documentation/CatalogName/CatalogName" URL (once for the catalog name and once for the article name) that I think people would find mildly surprising.

I searched for usages of "index.md" within documentation catalogs but almost all of the matches are for files that are the documentation extension of the module, so this wouldn't apply to those anyway. A refined search show 5 projects that have an "Index.md" / "index.md" file that isn't the documentation extension for the module.

"Apollo" in "apollographql"

"hummingbird-docs" in "hummingbird-project"

Already have @TechnologyRoot directives but those catalog also has a module so any root page file name logic wouldn't apply.

"Conformance" in "swift-protobuf"

"SwiftyESBuild" in "tuist"

"leetcode" in "rossmassey"

have modules in their catalogs so this root page file name logic wouldn't apply.

sofiaromorales · 2024-01-03T13:48:48Z

Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

+            nameMatch.value = Article(markup: nameMatch.value.markup, metadata: metadata, redirects: nameMatch.value.redirects, options: nameMatch.value.options)
+            registerRootPages(from: [nameMatch], in: bundle)
+        } else {
+            let path = NodeURLGenerator.Path.documentation(path: title).stringValue


Suggested change

let path = NodeURLGenerator.Path.documentation(path: title).stringValue

// Synthesize a minimal root with the same name as the catalog.

let path = NodeURLGenerator.Path.documentation(path: title).stringValue

sofiaromorales

Left minor comments about the code comments. Looks good to merge 👍

d-ronnqvist · 2024-01-04T08:26:08Z

@swift-ci please test

d-ronnqvist · 2024-01-04T08:33:47Z

@swift-ci please test

Make @TechnologyRoot optional in articles

526e2ec

d-ronnqvist requested a review from sofiaromorales December 21, 2023 17:37

sofiaromorales reviewed Jan 2, 2024

View reviewed changes

Remove error when a root page can't be synthesized

43ff6d4

d-ronnqvist force-pushed the optional-technology-root branch from 448ec15 to 43ff6d4 Compare January 2, 2024 18:33

sofiaromorales reviewed Jan 3, 2024

View reviewed changes

sofiaromorales approved these changes Jan 3, 2024

View reviewed changes

Add documentation comment for the new private function

29757fd

Update check for supported years

811faa5

d-ronnqvist merged commit 5f57823 into apple:main Jan 4, 2024
2 checks passed

d-ronnqvist deleted the optional-technology-root branch January 4, 2024 09:00

d-ronnqvist mentioned this pull request Jan 8, 2024

[5.10] Update allowed years in check-source script #791

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `@TechnologyRoot` optional in article-only documentation #778

Make `@TechnologyRoot` optional in article-only documentation #778

d-ronnqvist commented Dec 21, 2023

sofiaromorales Jan 2, 2024

d-ronnqvist Jan 2, 2024

sofiaromorales commented Jan 2, 2024 •

edited

d-ronnqvist commented Jan 2, 2024

sofiaromorales Jan 3, 2024

sofiaromorales Jan 3, 2024

d-ronnqvist Jan 3, 2024

sofiaromorales Jan 3, 2024

sofiaromorales left a comment

d-ronnqvist commented Jan 4, 2024

d-ronnqvist commented Jan 4, 2024

-    private func synthesizeArticleOnlyRootPage(articles: inout [DocumentationContext.SemanticResult<Article>], bundle: DocumentationBundle) {
+    /// Assigns or creates a top level page to the documentation.
+    ///
+    /// This method assigns a root to the only article in a single-article catalog,
+    /// to the article with the same file name as the catalog folder name,
+    /// or synthesizes a root with the same name as the catalog.
+    ///
+    /// - Parameters:
+    ///   - articles: Non-root articles from the catalog.
+    ///   - bundle: The bundle containing the articles.
+    private func synthesizeArticleOnlyRootPage(articles: inout [DocumentationContext.SemanticResult<Article>], bundle: DocumentationBundle) {

	} else if let nameMatchIndex = articles.firstIndex(where: { $0.source.deletingPathExtension().lastPathComponent == title }) {
	} else if let nameMatchIndex = articles.firstIndex(where: { $0.source.deletingPathExtension().lastPathComponent == title \|\| $0.source.deletingPathExtension().lastPathComponent == "index" }) {
	// This catalog has an article with the same name as the catalog itself, or an article named index, so we make that the root.

	let path = NodeURLGenerator.Path.documentation(path: title).stringValue
	// Synthesize a minimal root with the same name as the catalog.
	let path = NodeURLGenerator.Path.documentation(path: title).stringValue

Make @TechnologyRoot optional in article-only documentation #778

Make @TechnologyRoot optional in article-only documentation #778

Conversation

d-ronnqvist commented Dec 21, 2023

Summary

Dependencies

Testing

One article

Article with same name as catalog

Synthesized root page

Checklist

sofiaromorales Jan 2, 2024

Choose a reason for hiding this comment

d-ronnqvist Jan 2, 2024

Choose a reason for hiding this comment

sofiaromorales commented Jan 2, 2024 • edited

d-ronnqvist commented Jan 2, 2024

sofiaromorales Jan 3, 2024

Choose a reason for hiding this comment

sofiaromorales Jan 3, 2024

Choose a reason for hiding this comment

d-ronnqvist Jan 3, 2024

Choose a reason for hiding this comment

sofiaromorales Jan 3, 2024

Choose a reason for hiding this comment

sofiaromorales left a comment

Choose a reason for hiding this comment

d-ronnqvist commented Jan 4, 2024

d-ronnqvist commented Jan 4, 2024

Make `@TechnologyRoot` optional in article-only documentation #778

Make `@TechnologyRoot` optional in article-only documentation #778

sofiaromorales commented Jan 2, 2024 •

edited