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

Converter: allow converting a directory w/o .docc extension #585

Merged
merged 9 commits into from
Jun 14, 2023

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented May 10, 2023

Summary

When converting a directory that only has Markdown files in it, it feels redundant to rename the directory to have a .docc extension just for the sake of rendering those files. It would be great if DocC supported converting any directory regardless of its extension out of the box.

This new behavior is conditional on the newly introduced --allow-arbitrary-catalog-directories flag.

Dependencies

N/A

Testing

You can test this with the Swift compiler documentation at https://github.com/apple/swift/tree/main/docs, which has no .docc extension and is just a collection of Markdown files.

Steps:

  1. Run git clone https://github.com/apple/swift
  2. Run swift run docc convert --allow-arbitrary-catalog-directories --transform-for-static-hosting --output-path output swift/docs

Checklist

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

  • Added tests: only updated the existing test
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary: no new APIs were added to document.

@MaxDesiatov MaxDesiatov requested a review from franklinsch May 10, 2023 17:23
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@d-ronnqvist
Copy link
Contributor

I think this change in behavior could be considered "breaking" for integrations with build workflows when the project doesn't have a documentation catalog.

Before these changes, when the build workflow would call docc and pass the --additional-symbol-graph-dir but not pass any documentation catalog, DocC would build documentation for only the symbols. With these changes it's possible that DocC would discover additional markdown files throughout the project1 and include them in the built documentation. These files may not have been intended for the same audience as the documentation and it's possible that a CI/CD workflow could publish the new documentation pages without someone noticing they're there.

Footnotes

  1. This behavior, and what files are discovered, also depend on what the current directory is when the build workflow invokes docc.

@ethan-kusters
Copy link
Contributor

Since we just want the support here – could we start be adding an additional flag that allows for this and suppresses the diagnostic? Something like --allow-arbitrary-catalog-directories maybe. And we could emit that flag in the existing diagnostic to increase discoverability.

In general we don't expect people to use unidentified DocC catalogs because it breaks existing integration points with SwiftPM and Xcode. So removing the diagnostic by default seems like a worse UX for the average user in order to enable this completely reasonable, but more power-user feature.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@@ -18,6 +18,8 @@ public protocol DocumentationWorkspaceDataProvider {
/// use `UUID().uuidString` for the provider's identifier.
var identifier: String { get }

var allowArbitraryCatalogDirectories: Bool { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT this property is specific to LocalFileSystemDataProvider. I think it'd make be better to move it there or to a new protocol that it can conform to.

@@ -174,6 +174,9 @@ extension Docc {
@available(*, deprecated, message: "Doxygen support is now enabled by default.")
public var experimentalParseDoxygenCommands = false

@Flag(help: "Allow catalog directories without the `.docc` extension.")
var allowArbitraryCatalogDirectories = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we'd want to label this "experimental" until it's been discussed in the forums.

Copy link
Contributor

@d-ronnqvist d-ronnqvist May 31, 2023

Choose a reason for hiding this comment

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

To clarify; I don't it needs to be discussed very formally but it would be good to at least have some post that pitches adding this flag since we don't expect developers to monitor PRs

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov requested a review from d-ronnqvist June 2, 2023 11:53
Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

LGTM

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov merged commit 3dca83a into main Jun 14, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/convert-any-directory branch June 14, 2023 06:54
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