Minimally integrate per-page HTML content into each "index.html" file #1383
Conversation
|
@swift-ci please test |
|
@swift-ci please test |
patshaughnessy
left a comment
patshaughnessy
left a comment
There was a problem hiding this comment.
Does this solution consider the --no-transform-for-static-hosting (no index.html files at all) and --experimental-enable-custom-templates CLI options? Does the custom template option allow DocC users to change the contents of index.html? That might cause trouble in FileWritingHTMLContentConsumer.
Otherwise looks great!
| struct RenderedPageInfo { | ||
| /// The HTML content of the page as an XMLNode hierarchy. | ||
| /// | ||
| /// The string representation of those node hierarchy is intended to be inserted _somewhere_ inside the `<body>` HTML element. |
There was a problem hiding this comment.
| /// The string representation of those node hierarchy is intended to be inserted _somewhere_ inside the `<body>` HTML element. | |
| /// The string representation of this node hierarchy is intended to be inserted _somewhere_ inside the `<body>` HTML element. |
Also, should we split this doc comment up into an abstract and a note, or overview?
| return nil | ||
| } | ||
|
|
||
| let names: LinkedElement.Names |
There was a problem hiding this comment.
We could extract this passage about determining the names of the page into a separate function.
| // This symbol has multiple unique names | ||
| let titles = [SourceLanguage: String]( | ||
| titles.map { trait, title in | ||
| (trait.sourceLanguage ?? .swift, title) |
There was a problem hiding this comment.
Strictly speaking should we ignore traits that are not about source language? (Not sure if any such traits exist in practice?) I believe in a similar loop elsewhere you have a guard on the source language and a compactMap.
|
|
||
| // A helper function that transforms SymbolKit fragments into renderable identifier/decorator fragments | ||
| func convert(_ fragments: [SymbolGraph.Symbol.DeclarationFragments.Fragment]) -> [LinkedElement.SymbolNameFragment] { | ||
| func convert(kind: SymbolGraph.Symbol.DeclarationFragments.Fragment.Kind) -> LinkedElement.SymbolNameFragment.Kind { |
There was a problem hiding this comment.
Two nested functions with the same name is a bit confusing. Could the inner function be an extension on the SymbolGraph.Symbol.DeclarationFragments.Fragment.Kind type?
| } | ||
|
|
||
| // Join together multiple fragments of the same identifier/decorator kind to produce a smaller output. | ||
| var result: [LinkedElement.SymbolNameFragment] = [] |
There was a problem hiding this comment.
We could extract this part into a class method on the LinkedElement.SymbolNameFragment type, or extend Array<LinkedElement.SymbolNameFragment> maybe? Just trying to simplify this code and make it easier to read.
| // Title | ||
| let titleVariants = symbol.titleVariants.allValues.sorted(by: { $0.trait < $1.trait }) | ||
| for (trait, languageSpecificTitle) in titleVariants { | ||
| guard let language = trait.sourceLanguage else { continue } |
There was a problem hiding this comment.
This guard statement is what I was referring to earlier. We ignore traits here that don't refer to source language - should we do that above also?
| attributes = nil | ||
| } | ||
|
|
||
| hero.addChild( |
There was a problem hiding this comment.
It would be great to see an example of what the markup would look like for a symbol page with multiple titles. Since DocC Render doesn't render that normally, this would be covering new ground.
|
|
||
| // Note; this isn't a Comparable conformance so that it can remain private to this file. | ||
| private extension DocumentationDataVariantsTrait { | ||
| static func < (lhs: DocumentationDataVariantsTrait, rhs: DocumentationDataVariantsTrait) -> Bool { |
There was a problem hiding this comment.
Maybe add a generic map function here that you could use in various places above.
| init(data: Data) throws { | ||
| let content = String(decoding: data, as: UTF8.self) | ||
|
|
||
| // ???: Should we parse the content with XMLParser instead? If so, what do we do if it's not valid XHTML? |
There was a problem hiding this comment.
This strikes me as a bit dangerous. It's probably a safe assumption that each index.html file will contain these tags, but we shouldn't crash if they do not. Or if there are multiple <noscript> tags for some reason.
Could we check the ranges a bit more robustly, and skip the pages that have an unexpected index.html file? And maybe write a warning in this case?
Also we should handle the case if the index.html is missing entirely for some reason.
| ) throws { | ||
| self.targetFolder = targetFolder | ||
| self.fileManager = fileManager | ||
| self.htmlTemplate = try HTMLTemplate(data: fileManager.contents(of: htmlTemplate)) |
There was a problem hiding this comment.
If the file is missing, should we catch this Foundation error, and wrap it in a custom error that would emit an error message that is more specific and descriptive?
2 participants
Bug/issue #, if applicable: rdar://163326857
Summary
This is the 1st (of probably 2) integration slices of #1366
This PR adds a new
HTMLRendererthat can render articles and symbols intoHTMLRenderer/RenderedPageInfo.In order to access information about resolved pages, resolved assets, etc. it uses a private
ContextLinkProviderthat conforms to theLinkProviderprotocol in DocCHTML.This PR also adds a new
HTMLContentConsumerprotocol and a concreteFileWritingHTMLContentConsumerimplementation that embeds the per-page HTML content inside of the<noscript>tag of the index.html file that DocC would normally make an exact copy of for each page.The main integration of these new types happen in the
ConvertActionConsumer. If it is passed anHTMLContentConsumerit will create aHTMLRendererfor eachDocumentationNodethat it processes. This means that the output is both a JSON file with content and and HTML file with content.Notably missing from this PR is:
Dependencies
None.
Testing
Nothing in particular for this PR. It intentionally lacks the CLI feature flag that would allow this to be used in
docc convert. See #1366 for how it eventually does get used.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded