Fix nested type handling #62
Conversation
|
Thanks so much for taking the time to identify this issue and provide a fix, @victor-pavlychko. I really appreciate it! I'd been holding off on investing in a robust test suite for this functionality, because I thought it might soon be replaced (see #31). But now that I'm more confident that the current, syntactic approach will be sticking around, I'd like to add more unit tests to exercise these kinds of corner cases. Could you please add a test case to verify the behavior of your patch? The example you used here is great; just a few requested changes to match the conventions of the existing test: public class C { }
extension C {
public enum E {
case c
}
}
extension C.E {
public static let tp = 0
} |
| graph.aspectRatio = 0.125 | ||
| graph.center = true | ||
| graph.overlap = "compress" | ||
| guard sections.map(\.symbols.count).reduce(0, +) != 0 else { return "" } |
There was a problem hiding this comment.
I'm still on the fence about Swift's new keypath map syntax. But setting that aside: We don't need to compute the total count to determine if there are any symbols. A better way would be to do let _ = sections.first { !$0.symbols.isEmpty }.
There was a proble 10000 m hiding this comment.
I'm not sure about checking the first item only:
- I noticed that
sectionsare processed withfilter { !$0.symbols.isEmpty }so there should be no empty sections at the moment, we can just test forsections.isEmpty - at the same time, I see
guard !symbols.isEmpty else { return nil }in the generation code
So I decided to write a more defensive check to be sure that it won't break in the future.
I'd prefer to state that we never have empty sections and simplify/remove all relevant checks.
There was a problem hiding this comment.
To clarify: the use of first doesn't check the first item, but returns as soon as it finds a section with non-empty symbols. But taking another look, you're right: a simple check on sections.isEmpty should be sufficient. The other guard statement you mentioned was from an earlier iteration, and I neglected to remove it.
There was a problem hiding this comment.
Oh, right, I've misunderstood your initial suggestion. Thanks for clarifying that.
Changing to isEmpty + removed that guard statement.
| var graph = symbol.graph(in: module) | ||
| guard !graph.edges.isEmpty else { return "" } | ||
|
|
||
| graph.aspectRatio = 0.125 | ||
| graph.center = true | ||
| graph.overlap = "compress" | ||
| guard sections.map(\.symbols.count).reduce(0, +) != 0 else { return "" } | ||
|
|
||
| let algorithm: LayoutAlgorithm = graph.nodes.count > 3 ? .neato : .dot | ||
| var svg: HypertextLiteral.HTML? | ||
| var graph = symbol.graph(in: module) | ||
| if !graph.edges.isEmpty { | ||
| graph.aspectRatio = 0.125 | ||
| graph.center = true | ||
| graph.overlap = "compress" | ||
|
|
||
| let algorithm: LayoutAlgorithm = graph.nodes.count > 3 ? .neato : .dot | ||
|
|
||
| do { | ||
| svg = try HypertextLiteral.HTML(String(data: graph.render(using: algorithm, to: .svg), encoding: .utf8) ?? "") | ||
| } catch { | ||
| logger.error("\(error)") | ||
| do { | ||
| svg = try HypertextLiteral.HTML(String(data: graph.render(using: algorithm, to: .svg), encoding: .utf8) ?? "") | ||
| } catch { | ||
| logger.error("\(error)") | ||
| } |
There was a problem hiding this comment.
I think this has gotten complex enough to warrant extraction into a separate helper method / property (e.g. var svg: HyperTextLiteral.HTML. What do you think?
There was a problem hiding this comment.
Agree, moved to graphHTML property.
| } | ||
|
|
||
| return #""" | ||
| <section id="relationships"> | ||
| <h2 hidden>Relationships</h2> | ||
| <figure> | ||
| \#(svg ?? "") | ||
| \#(svg.flatMap { svg in |
There was a problem hiding this comment.
I'm not sure what flatMap is providing here as compared to the original, but that could just be me having trouble reading the diff.
There was a problem hiding this comment.
I moved the <figure> block inside flatMap so it is not rendered when the svg data is missing.
There was a problem hiding this comment.
Ah, got it. Thanks for clarifying.
Sources/SwiftDoc/Interface.swift
Outdated
| @@ -55,9 +55,9 @@ public final class Interface: Codable { | |||
| public private(set) lazy var relationships: [Relationship] = { | |||
| var relationships: Set<Relationship> = [] | |||
| for symbol in symbols { | |||
| let `extension` = symbol.context.compactMap({ $0 as? Extension }).first | |||
| let owner = symbol.context.last(where: { $0 is Extension || $0 is Symbol }) | |||
There was a problem hiding this comment.
I'm not sure owner is the right name for this. Can we come up with something better? (If not, we can go with something inelegant but precise, like lastExtensionOrSymbolContext)
There was a problem hiding this comment.
I can also propose lastDeclarationScope, what do you think?
|
Thanks for the quick feedback. I have added a test for |
|
@victor-pavlychko, I was going to work on the same thing but you beat me to it 😭
I was wondering the same thing. Regardless of whether it can or can't test executables, it might be worth moving all of this functionality to SwiftDoc library. |
|
I performed manual testing, the changes in this branch fix the issue #25 |
I'm still not 100% on the current shape and spelling of the SwiftDoc module, but I'm pretty confident that anything to do with rendering pages should be in a separate module. While we're waiting on end-to-end tests for the executable, could we write tests that |
Tried that: it complies swift files and then fails to link with the I suggest leaving those test methods disabled for now and enabling them once the functionality is moved somewhere. |
- rename owner -> lastDeclarationScope - improve checks in Relationships renderer
There was a problem hiding this comment.
Great work, @victor-pavlychko! Thanks for incorporating my feedback and answering my questions. I'm really happy to see this merged for the next release.
If you don't mind me asking for one more thing: Could you please update the Changelog to add an entry for this fix (sorry for not mentioning this until now; this is a new process I've been trying since #65, and am still figuring out how to document / automate everything)
Co-Authored-By: Mattt <mattt@me.com>
|
Perfect. Thanks again, @victor-pavlychko! 🎉 |
First of all, thanks for a great tool! 🙂
While running swift-doc with one of my current projects, I noticed a bug with nested type handling.
Consider the following input:
This snippet triggers the following issues:
CaseandVersymbols are mistakenly associated withOutersymbol