-
Notifications
You must be signed in to change notification settings - Fork 167
Experimental markdown output #1303
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
base: main
Are you sure you want to change the base?
Experimental markdown output #1303
Conversation
…ert feature flags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot to unpack here and I've only had the time to skim thought the code but these are a few things that stand out to me:
-
If this is intended as experimental feature, then it can't add actual
publicAPI because consumers of SwiftDocC would have no way of knowing what API is stable and what is experimental and could still have breaking changes or be removed.If a goal of this API is that clients can use it while it's still experimental then it needs to be marked as SPI (using
@_spi(SomeName)so that those clients make a deliberate decision to use it and acknowledge that it may have breaking changes. -
The added tests follow a pattern not seen elsewhere in the repo. I left some more specific comments about that.
Sources/SwiftDocC/SwiftDocCMarkdownOutput/MarkdownOutputManifest.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Model/MarkdownOutput/Translation/MarkdownOutputSemanticVisitor.swift
Show resolved
Hide resolved
Tests/SwiftDocCTests/Rendering/Markdown/MarkdownOutputTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/Rendering/Markdown/MarkdownOutputTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/Test Bundles/MarkdownOutput.docc/MarkdownOutput.symbols.json
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/Test Bundles/MarkdownOutput.docc/MarkdownOutput.symbols.json
Outdated
Show resolved
Hide resolved
…nt unneccessary diffs on re-run
Sources/SwiftDocC/Model/MarkdownOutput/Translation/MarkdownOutputSemanticVisitor.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Model/MarkdownOutput/Translation/MarkdownOutputSemanticVisitor.swift
Show resolved
Hide resolved
| var manifestRelationship: MarkdownOutputManifest.RelationshipSubType? { | ||
| // Structured like this to cause a compiler error if a new case is added | ||
| switch self { | ||
| case .conformingTypes: .conformingTypes | ||
| case .conformsTo: .conformsTo | ||
| case .inheritsFrom: .inheritsFrom | ||
| case .inheritedBy: .inheritedBy | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: what's the goal of this code? Should this enumeration be defined so that new callers need to handle unknown cases?
| /// The URI of the document | ||
| public let uri: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. maybe a better question is "what is a caller meant to do with this value"?
| } | ||
| } | ||
|
|
||
| public func children(of parent: Document) -> Set<Document> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that the added comment doesn't fully answer my question.
- What does it mean for a "document" to be the "parent" of another document?
- Is this based on the developer defined documentation hierarchy or is it based on the base symbol hierarchy?
- What is a caller meant to use this function for?
- If the caller is meant to process the document's hierarchically, would it make more sense to have the data also have a hierarchy?
| public let modules: [String] | ||
|
|
||
| public enum CodingKeys: String, CodingKey { | ||
| case kindDisplayName = "kind" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why not also use the more descriptive name in the file?
| public let introduced: String? | ||
| public let deprecated: String? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should clarify; I wasn't asking about how this is represented on the page or in the file. I was asking about the type of this property. That's why I asked what a caller is meant to do with the version information and if the caller is meant to inspect or process the information somehow.
| public var role: String? | ||
| public let uri: String | ||
| public var title: String | ||
| public let framework: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "location" and what do you mean by "document"? Is a "document" the same thing as a "documentation page"?
Assuming that a symbol is a kind of "document";
- if "SomeModule" extends a type in "ExtendedModule" to add a
doSomething()function. What's would be the "location" for that document? - if a cross import overlay between "ModuleA" and "ModuleB" defines a
doSomething()function. What's would be the "location" for that document?
Alternatively, if that distinction isn't relevant for this property, what is a caller meant to do with this information?
| /// One or more protocols to which a type conforms. | ||
| case conformsTo | ||
| /// One or more types that conform to a protocol. | ||
| case conformingTypes | ||
| /// One or more types that are parents of the symbol. | ||
| case inheritsFrom | ||
| /// One or more types that are children of the symbol. | ||
| case inheritedBy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a subset of the cases in Relationship.
Is there a reason for not including the other cases and is there a reason for not using the already existing type?
| return data | ||
| } | ||
| /// Data for this node to be rendered to disk as a markdown file. This method renders the metadata as a JSON header wrapped in an HTML comment block, then includes the document content. | ||
| public func generateDataRepresentation() throws -> Data { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is the client meant to do anything else with this raw data or is it only "be rendered to disk as a markdown file"?
Alternatively, is the client meant to write these files at all? I was sort of under the impression that it was DocC that created these files and the the client would only read them. If the client doesn't create and write its own files, does this need to be SPI or could it be package or internal access?
Sources/SwiftDocCUtilities/Action/Actions/Convert/JSONEncodingRenderNodeWriter.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Model/Rendering/RenderNode/RenderMetadata.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Model/MarkdownOutput/Translation/MarkdownOutputMarkdownWalker.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Model/MarkdownOutput/Translation/MarkdownOutputMarkdownWalker.swift
Outdated
Show resolved
Hide resolved
| import Markdown | ||
|
|
||
| /// Performs any markup processing necessary to build the final output markdown | ||
| internal struct MarkdownOutputMarkupWalker: MarkupWalker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI (not blocking): it's redundant to specify internal here because it's the default access level.
| } | ||
|
|
||
| func consume(markdownManifest: MarkdownOutputManifest) throws { | ||
| let url = targetFolder.appendingPathComponent("\(markdownManifest.title)-markdown-manifest.json", isDirectory: false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: what's the reason for including the title in the manifest file?
Have I misunderstood that archives would only have a single manifest file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One archive will only have a single manifest file, but if you are processing the output of multiple archives this allows you to distinguish them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the caller already know what archive the manifest belongs to based on what archive the manifest file is inside?
Sources/SwiftDocCUtilities/Action/Actions/Convert/JSONEncodingRenderNodeWriter.swift
Outdated
Show resolved
Hide resolved
| ## Overview | ||
| Nothing to see here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This is just noise (and just slightly malformed because the rendered page would have two "Overview" sections)
d-ronnqvist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There continues to be lot of API with a higher access level than what's needed and a handful of unused API.
Issue #1301
Summary
This PR addresses the changes proposed in #1301. It involves a new translator / semantic visitor / markup walker to create the new outputs from
DocumentationNode. This is done for each node, after the render JSON is generated.Dependencies
None
Testing
There are extensive unit tests in
MarkdownOutputTests.swift. Testers can also run the convert command against any catalog with the--enable-experimental-markdown-outputand--enable-experimental-markdown-output-manifestflags set.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded