-
Notifications
You must be signed in to change notification settings - Fork 116
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
Changes from 4 commits
0e0cc28
ea31679
0c6d03a
1a0da0e
a8a36fc
4e3f8da
a7c6a43
9b7cadf
2b5b454
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,59 @@ | ||||||||||||
/* | ||||||||||||
This source file is part of the Swift.org open source project | ||||||||||||
|
||||||||||||
Copyright (c) 2021-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 | ||||||||||||
@_spi(FileManagerProtocol) import SwiftDocCTestUtilities | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This SPI import isn't needed anymore after we moved the type to use
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||||||||||||
|
||||||||||||
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 | ||||||||||||
|
||||||||||||
### テスト | ||||||||||||
- <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) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reproduced this on linux docker image. swift-docc/Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift Lines 133 to 134 in a8a36fc
Because How do you think we should do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to fix that. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 In other words; I would expect the destination to have be 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 commentThe reason will be displayed to describe this comment to others. Learn more. I think it will be resolved here in same
However before reaching that, the destination is checked here (28 lines above that point) and swift-docc/Sources/SwiftDocC/Model/Rendering/RenderContentCompiler.swift Lines 133 to 134 in a8a36fc
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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>
"""), 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 commentThe reason will be displayed to describe this comment to others. Learn more. I got it! I did as you told me. 😃 |
||||||||||||
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") | ||||||||||||
} | ||||||||||||
} |
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.
nit: This is a new file
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 👌🏻