Fix warning when curating a module under a technology root (#735) #780
Conversation
| } | ||
|
|
||
| if !hasTechnologyRoot { | ||
| problems.append(Problem(diagnostic: Diagnostic(source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.ModuleCuration", summary: "Linking to \((link.destination ?? "").singleQuoted) from a Topics group in \(nodeReference.absoluteString.singleQuoted) isn't allowed", explanation: "The former is a module test, and modules only exist at the root"), possibleSolutions: [])) |
There was a problem hiding this comment.
There's an extra "test" in the explanation of this user facing diagnostic.
| problems.append(Problem(diagnostic: Diagnostic(source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.ModuleCuration", summary: "Linking to \((link.destination ?? "").singleQuoted) from a Topics group in \(nodeReference.absoluteString.singleQuoted) isn't allowed", explanation: "The former is a module test, and modules only exist at the root"), possibleSolutions: [])) | |
| problems.append(Problem(diagnostic: Diagnostic(source: source(), severity: .warning, range: range(), identifier: "org.swift.docc.ModuleCuration", summary: "Linking to \((link.destination ?? "").singleQuoted) from a Topics group in \(nodeReference.absoluteString.singleQuoted) isn't allowed", explanation: "The former is a module, and modules only exist at the root"), possibleSolutions: [])) |
There was a problem hiding this comment.
We don't need to add a new test bundle for this. These tests only need a module to link to, which almost all test bundles already have. For example, these tests could use the "SourceLocations" bundle (since it's very small)
| if path.count == 0 { | ||
| return false | ||
| } | ||
|
|
||
| let node = context.topicGraph.nodeWithReference(path[0]) | ||
| return node?.kind == .module && node?.kind.isSymbol == false |
There was a problem hiding this comment.
minor: Instead of checking count == 0 here you could check isEmpty (which is a bit easier to read) but since the first value is accessed below, it would be better the get the top of the path in the guard statement.
Since the node is also optional, that could also be checked in the same guard statement to remove the optional behaviors from the checks of the node's kind:
| if path.count == 0 { | |
| return false | |
| } | |
| let node = context.topicGraph.nodeWithReference(path[0]) | |
| return node?.kind == .module && node?.kind.isSymbol == false | |
| guard let root = path.first, let kind = context.topicGraph.nodeWithReference(root)?.kind else { | |
| return false | |
| } | |
| return kind == .module && !kind.isSymbol |
| # Another Awesome Project | ||
|
|
||
| Technology Root | ||
|
|
||
| @Metadata { | ||
| @TechnologyRoot | ||
| } | ||
|
|
||
| ## Overview | ||
|
|
||
| This shouldn't create a warning because it's a technologyRoot | ||
|
|
||
| ## Topics | ||
|
|
||
| ### Code Reference | ||
| - ``MyAwesomeApp`` | ||
|
|
There was a problem hiding this comment.
"Another Awesome Project" doesn't help the reader understand what this file is doing to the bundle and what's the expected effect of adding it. Also, there's some extra content that's not needed that makes it harder to spot the 2 relevant parts (the technology root and the curation).
I would recommend using a very brief title, like "Root" or "Root curating a module" and then use the abstract/summary for a slightly more detailed description:
| # Another Awesome Project | |
| Technology Root | |
| @Metadata { | |
| @TechnologyRoot | |
| } | |
| ## Overview | |
| This shouldn't create a warning because it's a technologyRoot | |
| ## Topics | |
| ### Code Reference | |
| - ``MyAwesomeApp`` | |
| # Root curating a module | |
| @Metadata { | |
| @TechnologyRoot | |
| } | |
| Curating a module from a technology root is allowed and doesn't raise a warning. | |
| ## Topics | |
| - ``MyAwesomeApp`` |
| This is a project to test that no errors are created when a technology root refers to a | ||
| module. | ||
|
|
||
| This should create a warning because it links to a module without being a technology root |
There was a problem hiding this comment.
This sentence contradicts the behavior of the test (XCTAssertEqual(crawler.problems.count, 0)). One of the two should change, depending on which is the expected behavior.
| for node in context.rootModules { | ||
| XCTAssertNoThrow(try crawler.crawlChildren(of: node, prepareForCuration: { _ in }, relateNodes: { _, _ in })) | ||
| } | ||
|
|
||
| XCTAssertEqual(crawler.problems.count, 0) |
There was a problem hiding this comment.
This is only testing that there are no crawling warnings, which is already covered by checking context.problems.isEmpty since the content is already crawled while building up the documentation context.
It would be good to also check that the crawling formed the expected relationship between the technology root and the module by asserting the expected value of context.pathsTo(/* the module reference */).
There was a problem hiding this comment.
I added a relationship check, but I'm not sure if it's the cleanest way to do so. Do you have any suggestions for how I could improve my code below?
guard let moduleNode = context.nodeWithSymbolIdentifier("SourceLocations"),
let pathToRoot = context.pathsTo(moduleNode.reference).first,
let root = pathToRoot.first else {
XCTFail("Module doesn't have technology root as a predecessor in its path")
return
}
XCTAssertEqual(root.path, "/documentation/Root")I'm unsure if manually asserting root.path with a literal string is wise.
There was a problem hiding this comment.
This looks good to me. Since the root path is determined based on a value that comes from this test I don't see a problem with asserting on it as a string literal.
| // when there's no warning we still curate the node | ||
| if childDocumentationNode.kind == .module { | ||
|
|
||
| var hasTechnologyRoot = documentationNode.kind.isSymbol == false |
There was a problem hiding this comment.
Since this isn't checking that the curating node is a module it is implicitly relying on the fact that articles aren't top-level pages which is a bit subtle. The two main ways to solve this would be
- Document the subtleties that this check relies on and describe why it produces the correct behavior
- Check the curating node the same as the first values in
context.pathsTo(...)
There was a problem hiding this comment.
Could you please expand on what you mean by this? I'm still new to this codebase and how it works, but would love to learn more about it.
There was a problem hiding this comment.
Absolutely. If I haven't mentioned it already, when we say "curation" we may organizing documentation into a hierarchy by writing a link to another page in a "Topics sections". For example, this excerpt from the @Metadata documentation curates a handful of other pages
## Topics
### Customizing the Presentation of a Page
- ``DisplayName``
- ``PageImage``
- ``PageKind``
...The way that DocC processes the curation happens in 4 steps;
- "crawling" the authored curation, starting with all top level symbols, nodes, and roots in order to relate nodes
- adding all the remaining "uncurated" symbols and articles based on the symbol hierarchy (and articles are added top-level)
- "crawling" any page that was added in step 2 since the crawler didn't reach that page in step 1.
- checking if step 3 related any nodes that was also related in step 2 and prefer the authored curation (3) over the inferred "automatic" curation (2).
The way that both of those "crawl" step works its that the DocumentationCurator is given a list of references and for each reference it recursively descends the authored curation (the links to other pages in the authored topic sections) and relates the linked nodes with the page that has the curation.
Because step 1 only descends from top-level symbols, modules, and technology root pages; the only way that the curator would visit an article was if that article was curated somewhere. If the curator doesn't visit the article in step 1, the article gets added top-level to the only module page or root page, as long as there's not more than once. If this happens the curator will visit the article in step 3.
This means that if the curator is visiting an article, it either reached that article by descending from a module or technology root in step 1, or the article got "automatically" curated under a module/root page in step 2 and was visited explicitly in step 3.
Because of this, checking var hasTechnologyRoot = documentationNode.kind.isSymbol == false can only be true in two cases
- The node is a technology root
- The node is an article that has to have a module/root page as the first values of its
context.pathsTo(...)(otherwise the curator wouldn't have reached the article)
So, in the article case, the check still does has the correct overall behavior but the reason why it's correct is very subtle.
One way to avoid this subtle but correct behavior would be to perform the same check on the current node and the first values in context.pathsTo(...). This could have a structure that looks something like this:
func isTechnologyRoot(_ reference: ResolvedTopicReference) -> Bool {
return // if the node for this reference is a technology root or not.
}
let hasTechnologyRoot = isTechnologyRoot(nodeReference) || context.pathsTo(nodeReference).contains(where: { path in
guard let maybeRoot = path.first else { return false }
return isTechnologyRoot(maybeRoot)
})There was a problem hiding this comment.
Thank you for taking the time to explain the curating for me. After reading this more times than I'll like to admit 😅, and looking through some of the code base. I was able to wrap my brain around what's going on!!
If I'm understanding correctly, DocC curating is similar to a topological sort on a directed graph. Where the top level symbols lead to the recursive curation of all references they related too. Then this process repeats for any uncharted symbols/articles.
|
Thank you for the feedback, I appreciate it! I was able to learn from it and will also be able to improve my code. I'm aiming to make the changes by this Sunday when I have time off from work. |
Change check during documentation curating that triggers a warning if a module is not a top-level page. To not trigger if the module has a technology root. In order, to give users more flexibility in creating their documentation hirerarchy Issue: 735
The two test created are testModuleUnderAncestorOfTechnologyRoot, and testModuleUnderTechnologyRoot. These test verify that technology roots, nor ancestors of technology roots generate errors when linking to a module. testCrawlDiagnostics() already verifies that regular articles generate an error when linking to modules. So creating that testcase is not needed. Issue: 735
- Fix extra "test" added in diagnostic warning for curating modules - Use SourceLocations test bundles instead of creating one - Change test article descriptions to add more context/information - Verify that relation is created between root and module in ancestor test - Change `hasTechnologyRoot` to not rely on subtle behavior
Check that relationship is created between technology root and module in testModuleUnderTechnologyRoot.
|
Apologies for the delay. I'll reply on Monday. |
|
No worries and thank you for letting me know. I hope you have a good weekend! |
There was a problem hiding this comment.
This looks good to me.
There's some minor code formatting; duplicate empty lines, inconsistent spacing around curly braces, and missing indentation of the inner if-statement (where the new problem is appended) but I don't view any of that as blocking. That can always be fixed the next time someone make a change to this code.
|
@d-ronnqvist Thank you for taking the time to guide me through this issue and also having patience with how long this task took me. I learned a lot about swift and swift-docs while working on this and am grateful for the help. I'm interested in contributing more to this project, but I don't really know where to start out. Do you have any suggestions for another beginner friendly issue that I could work on? I'm interested in working on #737, but I wanted to double check to see if there's anything else I could do that would benefit me more. Also, is there anything else that I have to complete for this pull request? This is my first time doing a submitting a pull request so I want to make that I'm not making mistakes. |
We have a few other open issues marked "good first issue" but if there's a particular area or feature that you're interested in we most likely help find a suitable issue in that area. If you're interested in #737 then we can make that work. The biggest difference with something like #737 is that there are many possible solutions and that it almost certainly involves some amount of new user facing syntax. As such, the first step would be to post in the Swift Forums about it so that the community can agree on what the new syntax should be before implementing it. In the post it'd be good to state that you're looking for feedback on what the user facing syntax should be. For this it could be good to present a few different syntax alternatives and briefly describe their tradeoffs. If you want to iterate on the forum post together you can send me a "personal message" in the Swift Forums.
The only remaining thing that you can do is to update the branch to be up-to-date with |
|
@swift-ci please test |
|
@swift-ci please test |
Bug/issue #735
Summary
This PR allows technology roots to reference a module without emitting a warning. To let users have more freedom with how they structure their documentation.
Dependencies
None
Testing
I ran a version of docc that had my changes on this project to check if errors were created.
I also added two unittest
testModuleUnderAncestorOfTechnologyRoot()andtestModuleUnderTechnologyRoot()that verifies a warning isn't created when curated a module under a technology root or an ancestor of a technology root.testCrawlDiagnostics()already checks that a warning is created when an article links to a module, so there wasn't a need to create a test case to verify that.To test my changes run all unit tests and build the project linked above with the PR's version of docc.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded