Added warning for multiple root pages #1170 #1177
Conversation
Issue "Warn when the documentation contains more than one root page swiftlang#1170" Added diagnostic for multiple root pages warning, root page count validation
There was a problem hiding this comment.
Thanks for opening this PR.
As far as I can tell, there's nothing calling this new code so I don't think it's currently doing anything.
It would be good to add a handful of tests to verify both when this warning is expected and also when it's not. For example:
- When a documentation build has symbol graph files for more than one module, there should be a warning about it. In this case it's not clear which is the primary root and which is an unexpected extra root.
- When a documentation build has a symbol graph file and a manual
@TechnologyRoot, there should be a warning about it. In this case the module (from the symbol graph file) is the primary root and any manual@TechnologyRootis an unexpected extra root.- If either page is curated (organized into the documentation hierarchy) as a member of the other, the hierarchy won't have more than one root page, so there shouldn't be a warning
- When a documentation build has two or more
@TechnologyRootpages, there should be a warning about it. In this case it's not clear which is the primary root and which is an unexpected extra root.- If there are additional uncurated non-root page articles, the warning shouldn't list them as found root pages
- If either page is curated (organized into the documentation hierarchy) as a member of the other, the hierarchy won't have more than one root page, so it's
When there are tests that verify that the warning is raised when it's expected it would be nice to refine the information in the diagnostics to make them more helpful to developers who encounter them. For example:
- It the warning is associated with a manual
@TechnologyRootpage, it would be nice if it was associated with the file for that article (you can find this information inDocumentationContext/documentLocationMap) and even nicer if it was also associated with the source range of the@TechnologyRootdirective markup in that file. - It would be nice if the diagnostic included a
DiagnosticNotewith the source and markup range for each extra root page to help the developer find the other root page files. - It would be nice if the diagnostic included some
Solutionitems that remove each@TechnologyRootdirective so that the developer can easily adopt these suggestions and solve the issue.
| } | ||
|
|
||
| extension DocumentationContext { | ||
| func processRootModules() throws { |
There was a problem hiding this comment.
I'm not sure that this code does anything. AFAICT there's nothing that ever calls this function, so this warning would never be raised.
| } | ||
|
|
||
| extension DocumentationContext { | ||
| private func validateRootPageCount() { |
There was a problem hiding this comment.
nit: As a stylistic thing, a private function that's only ever called by a one-line function (that only makes that function call) can just as well be inlined directly into that function. The same applies to multipleRootPagesWarning(roots:) above (even though it's not private it's only ever called exactly once).
| let primaryRoot = roots.first! | ||
| let additionalRoots = Array(roots.dropFirst()) |
There was a problem hiding this comment.
minor: This won't behave deterministically across different program executions. The roots value is based on TopicGraph/nodes which is a dictionary and doesn't have a stable order across program executions.
This indeterminism will be mildly annoying to developers who get different warnings when they re-run docc and it will make it difficult to write tests that expect the presence of these warnings (because the exact error message would be different each time).
| let roots = topicGraph.nodes.values.filter { node in | ||
| return node.kind == .article && parents(of: node.reference).isEmpty | ||
| }.map { $0.reference } |
There was a problem hiding this comment.
I'm not sure that this will behave correctly. I think that this could have false positives for uncurated articles (that aren't explicit root pages) because the presence of multiple root pages would disable automatic article curation.
There was a problem hiding this comment.
Also, this is only considering articles but it's also possible that one or more of the root pages could be different modules from the symbol graph files.
Like the description of #1170 says: it might be worth to check for these things separately since the diagnostic message would want to convey different information to the developer in each case:
Specifically, there are two possible situations that are worth warning about separately:
- The documentation inputs contain symbol graphs for more than one main module.
- The documentation inputs contain more than one root page. This could either happen if the developer defined a
@TechnologyRootfor a catalog with symbol information (which would already have a root page for the module page), or if the developer defined more than one@TechnologyRoot.For the first case there isn't really a source location to associate this warning with. For the second case, it would be nice if the warning was associated with the markup for the extra
@TechnologyRootdirective.
|
I greatly appreciate your thorough explanation of the problem statement. I will diligently work on this task and endeavor to fulfill the requirements as specified. thank you so much! |
Added support for diagnostic for multiple root pages warning, root page count validation
Having warnings about unexpected inputs would help developers notice issues and correct the inputs they are passing to DocC or other misconfigurations in their projects.
@d-ronnqvist