Improve handling of lists when exporting markdown#1543
Conversation
- Links in list items should get resolved to title + link (179135030) - Term lists shouldn't render with the word "term" in them (166128254)
d-ronnqvist
left a comment
There was a problem hiding this comment.
Hi, it appears that this is addressing two issues that aren't related to each other (and that seemingly doesn't modify the same lines of code). Can you please open one PR per issue so that it' easier to review the changes?
|
Regarding the "Testing" section of the PR description;
|
There was a problem hiding this comment.
Please use Swift Testing for all new tests. See the "Adding new tests" section of the contributing document for more details.
There was a problem hiding this comment.
I was planning to migrate all the tests to swift testing as a subsequent PR to this (now these?), to reduce noise - is that OK or would you prefer me to add another file with just the new tests in it?
There was a problem hiding this comment.
The contributing document offers some guidance on this; specifically that you can create a new test suite in (even in the same file) that contains only the Swift Testing test(s):
Updating existing tests
If you're updating an existing test case with additional logic, we appreciate if you also modernize that test while updating it, but we don't expect it. If the test case is part of a large file, you can create new test suite which contains just the test case that you're modernizing.
If you modernize an existing test case, consider not only the syntactical differences between Swift Testing and XCTest, but also if there are any Swift Testing features or other changes that would make the test case easier to read, maintain, or debug.
| Tests the appearance of nested lists | ||
|
|
||
| ## Overview | ||
|
|
||
| This is a list: |
There was a problem hiding this comment.
minor: This markup is never verified in the output and can be removed.
| Tests the appearance of nested lists | |
| ## Overview | |
| This is a list: |
|
|
||
| ## Topics | ||
|
|
||
| ### No more links | ||
|
|
||
| Empty section |
There was a problem hiding this comment.
minor: This markup is never verified in the output and can be removed.
| ## Topics | |
| ### No more links | |
| Empty section |
| This is a list of things that have links: | ||
|
|
||
| - You can use ``MarkdownSymbol`` to do interesting things | ||
| - You can't use other things |
There was a problem hiding this comment.
minor: This markup is never verified in the output and can be removed.
| - You can't use other things |
| ## Topics | ||
|
|
||
| ### No more links | ||
|
|
||
| Empty section |
There was a problem hiding this comment.
minor: This markup is never verified in the output and can be removed.
| ## Topics | |
| ### No more links | |
| Empty section |
| Tests the appearance of inline and linked lists | ||
|
|
||
| ## Overview | ||
|
|
||
| These are some interesting links that are in list items: | ||
|
|
There was a problem hiding this comment.
minor: This markup is never verified in the output and can be removed.
| Tests the appearance of inline and linked lists | |
| ## Overview | |
| These are some interesting links that are in list items: | |
| These are some interesting links that are in ordered list items: | ||
|
|
There was a problem hiding this comment.
minor: This markup is never verified in the output and can be removed.
| These are some interesting links that are in ordered list items: | |
| } | ||
|
|
||
| mutating func visitSymbolLink(_ symbolLink: SymbolLink) { | ||
| mutating func convertSymbolLink(_ symbolLink: SymbolLink) -> (link: any InlineMarkup, abstract: (any Markup)?)? { |
There was a problem hiding this comment.
minor: As far as I can tell, this is an implementation details that other types aren't expected to call.
Also, AFAICT this method doesn't mutate the walker.
| mutating func convertSymbolLink(_ symbolLink: SymbolLink) -> (link: any InlineMarkup, abstract: (any Markup)?)? { | |
| private func convertSymbolLink(_ symbolLink: SymbolLink) -> (link: any InlineMarkup, abstract: (any Markup)?)? { |
|
|
||
| mutating func visitLink(_ link: Link) { | ||
|
|
||
| mutating func convertLink(_ link: Link) -> (link: Link, abstract: (any Markup)?) { |
There was a problem hiding this comment.
minor: As far as I can tell, this is an implementation details that other types aren't expected to call.
Also, AFAICT this method doesn't mutate the walker.
| mutating func convertLink(_ link: Link) -> (link: Link, abstract: (any Markup)?) { | |
| private func convertLink(_ link: Link) -> (link: Link, abstract: (any Markup)?) { |
| return List(newItems) | ||
| } | ||
|
|
||
| mutating func convertListItem(_ item: ListItem) -> ListItem { |
There was a problem hiding this comment.
minor: As far as I can tell, this is an implementation details that other types aren't expected to call.
Also, AFAICT this method doesn't mutate the walker.
| mutating func convertListItem(_ item: ListItem) -> ListItem { | |
| private func convertListItem(_ item: ListItem) -> ListItem { |
There was a problem hiding this comment.
This method could also benefit from a brief comment explaining what it does and why. From the signature all I can tell is that it converts a list item to the same type of list item.
| var newComponents: [any InlineMarkup] = [] | ||
| for inlineChild in paragraph.inlineChildren { | ||
| if let link = inlineChild as? Link { | ||
| let (converted, _) = convertLink(link) | ||
| newComponents.append(converted) | ||
| } else if let symbolLink = inlineChild as? SymbolLink { | ||
| if let (converted, _) = convertSymbolLink(symbolLink) { | ||
| newComponents.append(converted) | ||
| } | ||
| } else { | ||
| newComponents.append(inlineChild) | ||
| } | ||
| } | ||
| newChildren.append(Paragraph(newComponents)) |
There was a problem hiding this comment.
minor: It might make this implementation easier to read if this was extracted to a private function (or even an inner function. That would make it clearer at a glance that the outer loop is switching over the element type and calling different convert functions for each type.
| var newChildren: [any BlockMarkup] = [] | ||
| for child in item.blockChildren { | ||
| if let nestedList = child as? any ListItemContainer { | ||
| let converted = convertList(nestedList) | ||
| newChildren.append(converted) | ||
| } else if let paragraph = child as? Paragraph { | ||
| var newComponents: [any InlineMarkup] = [] | ||
| for inlineChild in paragraph.inlineChildren { | ||
| if let link = inlineChild as? Link { | ||
| let (converted, _) = convertLink(link) | ||
| newComponents.append(converted) | ||
| } else if let symbolLink = inlineChild as? SymbolLink { | ||
| if let (converted, _) = convertSymbolLink(symbolLink) { | ||
| newComponents.append(converted) | ||
| } | ||
| } else { | ||
| newComponents.append(inlineChild) | ||
| } | ||
| } | ||
| newChildren.append(Paragraph(newComponents)) | ||
| } else { | ||
| newChildren.append(child) | ||
| } | ||
| } |
There was a problem hiding this comment.
There are a couple patterns hiding in this implementation that I feel make it harder to read than necessary.
Both loops behave like map (but is slower because it doesn't preemptively reserve capacity for the known number of elements) and both if-else expressions behave like a switch over a value.
It might be easier to read this implementation if it leveraged those language constructs.
Bug/issue #, if applicable:
179135030
Summary
Ordered and unordered lists were getting directly output using format() unless we were processing a linked list (like a topics section). This bypassed the link resolution / formatting steps that happen when visiting a link.
Changed the processing of lists to find and convert any links or symbol links found in the list item.
Dependencies
N/A
Testing
Create source documentation with article and symbol links inside ordered or unordered lists, and within standard paragraphs of prose. Run
converton the documentation with the--enable-experimental-markdown-outputflag enabled. Examine the markdown produced in the docc archive. The links should be formatted the same in the list output as they are in the normal paragraph output.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded