-
Notifications
You must be signed in to change notification settings - Fork 126
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
Fix a doc anchor is not encoded correctly #867
Merged
+85
−2
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
0e0cc28
Fix a doc anchor is not encoded correctly
hironytic ea31679
Add a test for rendering heading anchors
hironytic 0c6d03a
Pass stored unencoded fragment
hironytic 1a0da0e
Remove the unsuitable test case
hironytic a8a36fc
Apply suggestions from code review
hironytic 4e3f8da
Treat a link destination as the "authored link"
hironytic a7c6a43
Revert "Treat a link destination as the "authored link""
hironytic 9b7cadf
Mark the test article as a technology root
hironytic 2b5b454
Merge branch 'main' into fix-doc-anchor
d-ronnqvist File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
Copyright (c) 2024 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
||
See https://swift.org/LICENSE.txt for license information | ||
See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
import Foundation | ||
import XCTest | ||
@testable import SwiftDocC | ||
import SwiftDocCTestUtilities | ||
|
||
class HeadingAnchorTests: XCTestCase { | ||
func testEncodeHeadingAnchor() throws { | ||
let catalogURL = try createTempFolder(content: [ | ||
Folder(name: "unit-test.docc", content: [ | ||
TextFile(name: "Root.md", utf8Content: """ | ||
# My root page | ||
|
||
This single article defines two headings and links to them | ||
|
||
@Metadata { | ||
@TechnologyRoot | ||
} | ||
|
||
### テスト | ||
- <doc:#テスト> | ||
|
||
### Some heading | ||
- <doc:#Some-heading> | ||
"""), | ||
]) | ||
]) | ||
let (_, bundle, context) = try loadBundle(from: catalogURL) | ||
|
||
let reference = try XCTUnwrap(context.soleRootModuleReference) | ||
let node = try context.entity(with: reference) | ||
let renderContext = RenderContext(documentationContext: context, bundle: bundle) | ||
let converter = DocumentationContextConverter(bundle: bundle, context: context, renderContext: renderContext) | ||
let renderNode = try XCTUnwrap(converter.renderNode(for: node, at: nil)) | ||
|
||
// Check heading anchors are encoded | ||
let contentSection = try XCTUnwrap(renderNode.primaryContentSections.first as? ContentRenderSection) | ||
let headings: [RenderBlockContent.Heading] = contentSection.content.compactMap { | ||
if case .heading(let heading) = $0 { | ||
return heading | ||
} else { | ||
return nil | ||
} | ||
} | ||
XCTAssertEqual(headings[0].anchor, "%E3%83%86%E3%82%B9%E3%83%88") | ||
XCTAssertEqual(headings[1].anchor, "Some-heading") | ||
|
||
// Check links to them | ||
let testTopic0 = try XCTUnwrap(renderNode.references["doc://unit-test/documentation/Root#%E3%83%86%E3%82%B9%E3%83%88"] as? TopicRenderReference) | ||
XCTAssertEqual(testTopic0.url, "/documentation/root#%E3%83%86%E3%82%B9%E3%83%88") | ||
let testTopic1 = try XCTUnwrap(renderNode.references["doc://unit-test/documentation/Root#Some-heading"] as? TopicRenderReference) | ||
XCTAssertEqual(testTopic1.url, "/documentation/root#Some-heading") | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This line fails in the CI
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.
Oh I have no idea. The test passes on my local Mac environment. But the test in the CI fails on both Mac and Linux.
The failed line is based on that you told me here. Can the key of
renderNode.references
have different values depending on the environment?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.
Reproduced this on linux docker image.
Currently all what I know is that the link which has
doc:#テスト
in its destination, is dropped by this guard. But it is not dropped on my local macOS.https://github.com/apple/swift-docc/blob/a8a36fcdd0f97210c8708f88466200f6d31b42e9/Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift#L133-L134
Because
URL(string: "doc:#テスト"))
returns nil on linux docker image and returns an object on local macOS.But the result of the CI suggests it can also happen on macOS under certain conditions.
How do you think we should do?
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.
I tried to fix that.
To my understanding, we should use
ValidatedURL(parsingAuthoredLink:)
rather thanURL(string:)
to get a url of the link destination because it is author provided documentation one.ValidatedURL(parsingAuthoredLink:)
is that already used byresolveTopicReference(_:)
which is called invisitLink(_:)
.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.
Hm.. I would have expected that by the time the link is rendered it would have already been "resolved" which should replace the authored destination with the
url
value from theResolvedTopicReference
. 🤔In other words; I would expect the destination to have be
"doc://unit-test/documentation/Root#%E3%83%86%E3%82%B9%E3%83%88"
by this point instead of"doc:#テスト"
.I wonder if there's a code path that doesn't do that. 🤔
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.
I think it will be resolved here in same
visitLink(_:)
https://github.com/apple/swift-docc/blob/a8a36fcdd0f97210c8708f88466200f6d31b42e9/Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift#L162
However before reaching that, the destination is checked here (28 lines above that point) and
URL(string:)
fails.https://github.com/apple/swift-docc/blob/a8a36fcdd0f97210c8708f88466200f6d31b42e9/Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift#L133-L134
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 doing that investigative work.
It looks like you've found another, unrelated, bug where links in synthesized root pages don't resolve their links in the intended manner. Unless you really want to, I don't think it's necessary to fix both bugs in the same PR.
This other bug is rather specific to links in synthesized root pages so it has a smaller impact and could be addressed separately. Anchors would still be encoded correctly—with your changes—in all other sources of documentation.
To get the test to pass you can explicitly mark it as a technology root (instead of relying on DocC to infer it as a root). To do this you'd add a
@TechnologyRoot
directive in the markup:If you do this you can revert the change to treat link destinations as authored during rendering. Links are intended to always be resolved before rendering happens and I would prefer to not add any more code that relies on resolving links during rendering.
If you're curious I can go into more detail on Friday or next week.
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.
I got it! I did as you told me. 😃