Add a new target for rendering Markdown content into static HTML #1369
Conversation
|
@swift-ci please test |
|
@swift-ci please test |
rdar://163326857
d-ronnqvist
force-pushed
the
output-html-1
branch
from
210a048 to
983a415
Compare
patshaughnessy
left a comment
patshaughnessy
left a comment
There was a problem hiding this comment.
Amazing work!
I left some coding nitpicks but this looks great. Maybe add some comments with examples of the HTML that each visit function generates. Even though you have that in the tests, it would be nice to see a simple example just in front of each function I think.
| .product(name: "Markdown", package: "swift-markdown"), | ||
| .target(name: "SwiftDocCTestUtilities"), | ||
| ], | ||
| swiftSettings: [.swiftLanguageMode(.v6)] |
There was a problem hiding this comment.
Do we need language mode v6 to support Swift Testing?
There was a problem hiding this comment.
No, we just need to specify swift-tools-version:6.0 in the package manifest which we already do. Because this code doesn't load any symbols or documentation catalogs, it can start using Swift Testing right away. Most other tests in DocC probably need the updated test helpers in #1362.
My reason for using the Swift 6 language mode here is that it enabled a number of features by default and is strict about concurrency warnings. That way, any new code has to follow the stricter concurrency checks from the start, rather than adapting it afterwards.
| func pathForSymbolID(_ usr: String) -> URL? | ||
|
|
||
| /// Provide information about an asset, or `nil` if the asset can't be found. | ||
| func assetNamed(_ assetName: String) -> LinkedAsset? |
There was a problem hiding this comment.
What are assets? Images? Videos? Both? Other objects? Maybe a bit of explanation here or where you define LinkedAsset below.
| } | ||
| } | ||
|
|
||
| package struct LinkedAsset { |
There was a problem hiding this comment.
See the previous comment. Should this be named LinkedImages ? Or, if we expect there to be videos or other types of media is having a single images attribute incorrect?
There was a problem hiding this comment.
Videos and I think downloadable files would also be considered assets. I'll update this comment and add a TODO for a future PR to verify that videos and downloads work, but that probably requires support for the @Video directive and @CallToAction directive first.
| self.linkProvider = linkProvider | ||
| } | ||
|
|
||
| func visit(_ paragraph: Paragraph) -> XMLNode { |
There was a problem hiding this comment.
I realized you have extensive unit tests with detailed examples, but it would be very helpful to paste an example here in a comment above each visit function to indicate what it produces. This one is trivial but the following functions are quite complex and hard to understand here.
There was a problem hiding this comment.
I added examples to the various visit(_:) functions and reformatted the tests to match in e0becf0
| return .element(named: "h\(level)", children: content) | ||
|
|
||
| case .richness: | ||
| let id = urlReadableFragment(plainTextTitle().lowercased()) |
There was a problem hiding this comment.
Should the call to lowercased() happen inside of urlReadableFragment?
There was a problem hiding this comment.
Because the urlReadableFragment comes from elsewhere in DocC I'd rather have it be exactly the same.
I wonder if we even need to lowercase the fragment/anchor here. I was trying to match what DocC Render does, but it only lowercases anchors to known sections like "parameters" or "topics" whereas authored headings don't get a lowercased anchor.
There was a problem hiding this comment.
I stopped lowercasing the anchors in 922e279
| attributes: ["href": destination.absoluteString] | ||
| ) | ||
| } else { | ||
| // If this is an unresolved documentation link, try to display only the name of the linked symbol; without the rest of its path and without its disambiguation |
There was a problem hiding this comment.
YES THANK YOU for not rendering "doc:"
| return children | ||
| } | ||
|
|
||
| var elements = Array(container) |
There was a problem hiding this comment.
Maybe extract the following code into a separate function. It's quite complex and performing a specific operation related to inline HTML. Extracting a function would also give you a chance to name this functionality, and a place to add some comments explaining what this is doing and why.
There was a problem hiding this comment.
For now I added a number of code comments and a FIXME in 519274b
| continue | ||
| } | ||
|
|
||
| // Gradually increase the content to try and parse |
There was a problem hiding this comment.
Can you extract the following inner loop into a separate function also? The outer/inner labels are quite confusing. Is there a simpler way to express this algorithm?
| .union(CharacterSet(charactersIn: "`")) // Also consider back-ticks as punctuation. They are used as quotes around symbols or other code. | ||
| .subtracting(CharacterSet(charactersIn: "-")) // Don't remove hyphens. They are used as a whitespace replacement. | ||
| static let whitespaceAndDashes = CharacterSet.whitespaces | ||
| .union(CharacterSet(charactersIn: "-–—")) // hyphen, en dash, em dash |
There was a problem hiding this comment.
Are these different characters here in the Swift source file? I think I see two but not sure. Is there another way to write this?
There was a problem hiding this comment.
This code is taken from elsewhere in DocC. In the future we might remove the duplication by either moving this functionality down into the Common target or adding it to the LinkProvider protocol so that the renderer can ask the calling code to create the anchor.
Regarding alternative ways to write this, we could either do 3 separate unions:
.union(CharacterSet(charactersIn: "-")) // hyphen
.union(CharacterSet(charactersIn: "–")) // en dash
.union(CharacterSet(charactersIn: "—")) // em dashor we could use unicode literals for each character so that it's clear that there are 3:
.union(CharacterSet(charactersIn: "\u{2010}\u{2013}\u{2014}")) // hyphen, en dash, em dash| return result | ||
| } | ||
|
|
||
| /// Returns the language specific symbol names sorted by the language. |
There was a problem hiding this comment.
Why is this not a method of LinkedElement.Names?
There was a problem hiding this comment.
Because later PRs call this for many non-name values, for example [SourceLanguage: [ParameterInfo]], [SourceLanguage: [any Markup]], and [SourceLanguage: [DeclarationFragment]]
|
@swift-ci please test |
|
@swift-ci please test |
2 participants
Bug/issue #, if applicable: rdar://163326857
Summary
This is the first broken down slice of #1366
Follow up PRs will incrementally add other changes that are already present in #1366
This adds a new internal library target for rendering Markdown content into static HTML.
Unlike a generic Markdown to HTML renderer that might be a good fit for Swift Markdown itself, this renderer has a
LinkProviderthat it can query for information about linked pages (such as their title and type of page) which it uses to render links and symbol links.The rendered output can either focus on "richness" or on "conciseness". Most of the code is the same for both, but a few code paths diverge:
<wbr/>elements into symbol names into semantically meaningful places to have a nicer word wrapping on the rendered page.The core of the HTML renderer is based on Foundation's XMLNode. The main reason for this is that it doesn't add any new dependencies which makes it significantly easier to get it into the toolchain. This doesn't add any public API, so we are free to completely rewrite the core rendered to use either an external dependency or a new library that we write ourselves without risking any breaking changes.
Dependencies
None.
Testing
Nothing in particular for this PR. It only adds an internal library target that's not used yet. See #1366 for how it does eventually get used.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded