-
Notifications
You must be signed in to change notification settings - Fork 150
Add warnings for multiple root pages #1276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add warnings for multiple root pages #1276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR.
The code generally looks good but there's a third case that's also worth warning about, because it's likely going to be the least intuitive to people;
if the documentation contains any symbols (has one module) but also has any @TechnologyRoot
pages it results in the same unsupported setup with multiple roots of the documentation hierarchy. The Solution
to suggest here is to remove each @TechnologyRoot
so that those pages are treated as articles under the module.
"symbols": [ | ||
{ | ||
"identifier": { | ||
"precise": "ModuleA", | ||
"interfaceLanguage": "swift" | ||
}, | ||
"names": { | ||
"title": "ModuleA", | ||
"navigator": null, | ||
"subHeading": null, | ||
"prose": null | ||
}, | ||
"pathComponents": ["ModuleA"], | ||
"docComment": null, | ||
"accessLevel": "public", | ||
"kind": { | ||
"identifier": "module", | ||
"displayName": "Module" | ||
}, | ||
"mixins": {} | ||
} | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: The module isn't a defined symbol in the file. Instead DocC creates the module symbol when it processes the rest of the symbol graph data.
The correct way do define a minimal (empty) symbol graph file would be "symbols": [],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it here: ded8478
"metadata": { | ||
"formatVersion": { | ||
"major": 1, | ||
"minor": 0, | ||
"patch": 0 | ||
}, | ||
"generator": "unit-test" | ||
}, | ||
"module": { | ||
"name": "ModuleA", | ||
"platform": { | ||
"architecture": "x86_64", | ||
"vendor": "apple", | ||
"operatingSystem": { | ||
"name": "macosx", | ||
"minimumVersion": { | ||
"major": 10, | ||
"minor": 15 | ||
} | ||
}, | ||
"environment": null | ||
} | ||
}, | ||
"symbols": [ | ||
{ | ||
"identifier": { | ||
"precise": "ModuleA", | ||
"interfaceLanguage": "swift" | ||
}, | ||
"names": { | ||
"title": "ModuleA", | ||
"navigator": null, | ||
"subHeading": null, | ||
"prose": null | ||
}, | ||
"pathComponents": ["ModuleA"], | ||
"docComment": null, | ||
"accessLevel": "public", | ||
"kind": { | ||
"identifier": "module", | ||
"displayName": "Module" | ||
}, | ||
"mixins": {} | ||
} | ||
], | ||
"relationships": [] | ||
} | ||
"""), | ||
|
||
// Second module symbol graph | ||
TextFile(name: "ModuleB.symbols.json", utf8Content: """ | ||
{ | ||
"metadata": { | ||
"formatVersion": { | ||
"major": 1, | ||
"minor": 0, | ||
"patch": 0 | ||
}, | ||
"generator": "unit-test" | ||
}, | ||
"module": { | ||
"name": "ModuleB", | ||
"platform": { | ||
"architecture": "x86_64", | ||
"vendor": "apple", | ||
"operatingSystem": { | ||
"name": "macosx", | ||
"minimumVersion": { | ||
"major": 10, | ||
"minor": 15 | ||
} | ||
}, | ||
"environment": null | ||
} | ||
}, | ||
"symbols": [ | ||
{ | ||
"identifier": { | ||
"precise": "ModuleB", | ||
"interfaceLanguage": "swift" | ||
}, | ||
"names": { | ||
"title": "ModuleB", | ||
"navigator": null, | ||
"subHeading": null, | ||
"prose": null | ||
}, | ||
"pathComponents": ["ModuleB"], | ||
"docComment": null, | ||
"accessLevel": "public", | ||
"kind": { | ||
"identifier": "module", | ||
"displayName": "Module" | ||
}, | ||
"mixins": {} | ||
} | ||
], | ||
"relationships": [] | ||
} | ||
"""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a makeSymbolGraph(moduleName:platform:symbols:relationships:)
helper that mosts tests use. It makes the test significantly shorter and easier to read and helps with data correctness as well compared to spelling out the raw JSON string.
TextFile(name: "ModuleA.symbols.json", utf8Content: """ | |
{ | |
"metadata": { | |
"formatVersion": { | |
"major": 1, | |
"minor": 0, | |
"patch": 0 | |
}, | |
"generator": "unit-test" | |
}, | |
"module": { | |
"name": "ModuleA", | |
"platform": { | |
"architecture": "x86_64", | |
"vendor": "apple", | |
"operatingSystem": { | |
"name": "macosx", | |
"minimumVersion": { | |
"major": 10, | |
"minor": 15 | |
} | |
}, | |
"environment": null | |
} | |
}, | |
"symbols": [ | |
{ | |
"identifier": { | |
"precise": "ModuleA", | |
"interfaceLanguage": "swift" | |
}, | |
"names": { | |
"title": "ModuleA", | |
"navigator": null, | |
"subHeading": null, | |
"prose": null | |
}, | |
"pathComponents": ["ModuleA"], | |
"docComment": null, | |
"accessLevel": "public", | |
"kind": { | |
"identifier": "module", | |
"displayName": "Module" | |
}, | |
"mixins": {} | |
} | |
], | |
"relationships": [] | |
} | |
"""), | |
// Second module symbol graph | |
TextFile(name: "ModuleB.symbols.json", utf8Content: """ | |
{ | |
"metadata": { | |
"formatVersion": { | |
"major": 1, | |
"minor": 0, | |
"patch": 0 | |
}, | |
"generator": "unit-test" | |
}, | |
"module": { | |
"name": "ModuleB", | |
"platform": { | |
"architecture": "x86_64", | |
"vendor": "apple", | |
"operatingSystem": { | |
"name": "macosx", | |
"minimumVersion": { | |
"major": 10, | |
"minor": 15 | |
} | |
}, | |
"environment": null | |
} | |
}, | |
"symbols": [ | |
{ | |
"identifier": { | |
"precise": "ModuleB", | |
"interfaceLanguage": "swift" | |
}, | |
"names": { | |
"title": "ModuleB", | |
"navigator": null, | |
"subHeading": null, | |
"prose": null | |
}, | |
"pathComponents": ["ModuleB"], | |
"docComment": null, | |
"accessLevel": "public", | |
"kind": { | |
"identifier": "module", | |
"displayName": "Module" | |
}, | |
"mixins": {} | |
} | |
], | |
"relationships": [] | |
} | |
"""), | |
JSONFile(name: "ModuleA.symbols.json", content: makeSymbolGraph(moduleName: "ModuleA")), | |
JSONFile(name: "ModuleB.symbols.json", content: makeSymbolGraph(moduleName: "ModuleB")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed here: agisilaos@ded8478
// Verify each warning has a solution to remove the TechnologyRoot directive | ||
for problem in multipleRootsProblems { | ||
XCTAssertEqual(problem.possibleSolutions.count, 1) | ||
let solution = problem.possibleSolutions.first! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: If this is nil
it will trap and stop running the remainder of the tests. A more defensive solution would be to use XCTUnwrap
to handle the nil
value. This will gracefully fail the tests by reporting a test failure without interrupting other tests.
let solution = problem.possibleSolutions.first! | |
let solution = try XCTUnwrap(problem.possibleSolutions.first) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here: aa33e18
This is the third root page. | ||
"""), | ||
|
||
InfoPlist(displayName: "TestBundle", identifier: "com.test.example"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI (non-blocking): This Info.plist file hasn't been needed for some time. I understand that you looked at other tests in this file and followed what they were doing (which they likely did for historical reasons) but this test (and the other test) would work the same without this file because the test doesn't verify any of the information that the Info.plist file configures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here: 4ec94c0
XCTAssertTrue(multipleModulesProblem.diagnostic.explanation?.contains("ModuleA, ModuleB") == true) | ||
|
||
// Verify the warning doesn't have a source location since it's about the overall input structure | ||
XCTAssertNil(multipleModulesProblem.diagnostic.source) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor (non-blocking): We could refer to one of the symbol graph files here and use DiagnosticNote
values to help the developer find the other symbol graph files so that this diagnostic becomes a bit more actionable.
See for example DocumentationContext.emitWarningsForSymbolsMatchedInMultipleDocumentationExtensions(with:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here: 121c159
When documentation contains symbols (from symbol graphs) and also has @TechnologyRoot pages, emit a warning about the unsupported setup with multiple roots in the documentation hierarchy. The solution suggests removing the @TechnologyRoot directive so pages are treated as articles under the module instead.
Use makeSymbolGraph() helper instead of raw JSON strings for cleaner and more maintainable test code, as suggested in PR review.
Use XCTUnwrap() instead of force unwrapping to handle potential nil values more gracefully and prevent test traps, as suggested in PR review.
Remove Info.plist file from testWarnsAboutMultipleTechnologyRootDirectives as it's not needed for this test, per PR review feedback.
Enhance the "symbols with @TechnologyRoot pages" warning by adding DiagnosticNote entries that point to the symbol graph files causing the multiple roots issue. This helps developers identify exactly which symbol graph files are part of the problem. Also update the test to verify the diagnostic notes are present. Based on PR review feedback.
@d-ronnqvist Addressed your comment in this commit: 665321f
Thank you for the thorough review! Looking forward to contribute even further in this project! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this code is checking for all 3 scenarios, there are 2 things that I notice:
- The 3 checks and diagnostics are fairly similar, both conceptually (diagnosing multiple root pages of some sort) and in code (doing similar checks and creating similar diagnostics and solutions).
- It's odd to me that these 3 checks happen in 3 different places.
It would be good to address both of these by extracting all 3 into a common place (for example private func emitWarningsForMultipleRootPages() {...}
) and reduce some of that duplication.
let symbolsWithRootProblems = context.problems.filter { $0.diagnostic.identifier == "org.swift.docc.TechnologyRootWithSymbols" } | ||
XCTAssertEqual(symbolsWithRootProblems.count, 2, "Should emit warnings for both @TechnologyRoot directives when symbols are present") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this code is checking for all 3 scenarios of multiple root pages, this test assertion becomes a good example of why it's important to not ignore unexpected diagnostics in tests.
The way the code is written right now, this setup would result in both "MultipleTechnologyRoots" warnings and "TechnologyRootWithSymbols" warnings for the same @TechnologyRoot
directives but because the test filters the problems to a smaller subset it misses these warnings and because of this doesn't reflect the experience that a real user would have in this scenario.
Fixes: #1170
Summary
This PR implements warnings when DocC documentation contains more than one root page, addressing unexpected input configurations that may lead to unexpected behaviors.
Expected User Experience:
When developers call
docc convert
directly with custom inputs that contain multiple root pages (either multiple main modules from symbol graphs or multiple@TechnologyRoot
directives), DocC will now emit helpful warnings to alert them about the unexpected configuration.Implementation Overview:
DocumentationContext.swift
to detect multiple main modules during symbol graph processing@TechnologyRoot
directives after article processingDependencies
No external dependencies. This is a self-contained enhancement to the existing DocC codebase.
Testing
Steps:
Setup Instructions:
feature/multiple-root-page-warnings
branchswift test --filter DocumentationContext_RootPageTests
to verify the new tests passTesting Multiple TechnologyRoot Directives:
.docc
catalog with multiple articles containing@TechnologyRoot
directivesdocc convert
on the catalog@TechnologyRoot
directive with source location and fix suggestionsTesting Multiple Main Modules:
.docc
catalog with multiple symbol graph files for different modulesdocc convert
on the catalogTest Content:
The test files in
Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContext+RootPageTests.swift
provide examples of both scenarios:testWarnsAboutMultipleTechnologyRootDirectives()
- Tests multiple@TechnologyRoot
directivestestWarnsAboutMultipleMainModules()
- Tests multiple main modules from symbol graphsChecklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded