-
Notifications
You must be signed in to change notification settings - Fork 158
Add a new version of the communication protocol between DocC and external link resolver executables #1292
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
Add a new version of the communication protocol between DocC and external link resolver executables #1292
Conversation
@swift-ci please test |
… capabilities rdar://159610082 swiftlang#802
rdar://76656957 rdar://77906558 rdar://86334416 rdar://92254059 rdar://120424365 swiftlang#341
14a4f88
to
dfc61c4
Compare
@swift-ci please test |
This test failure happens on other PRs as well. I opened #1293 to fix it. |
@swift-ci please test |
@swift-ci please test |
1 similar comment
@swift-ci please test |
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.
The code changes look correct to me, and I didn't spot any issues or bugs in the implementation. I left some comments on areas where it was hard for me to understand why some of the changes were made.
The way the commits were organised with multiple changes within one commit made it hard for me to review and ensure there were no regressions in behaviour being introduced; I would have found it helpful to have longer commit descriptions and/or smaller commits.
Thanks!
...ocC/Infrastructure/External Data/OutOfProcessReferenceResolver+DeprecatedCommunication.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolver+NavigatorIndex.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Link Resolution/LinkResolver.swift
Outdated
Show resolved
Hide resolved
for case let dependency as TopicRenderReference in (entity.references ?? []) { | ||
guard let url = URL(string: dependency.identifier.identifier), let rawBundleID = url.host else { | ||
// This dependency doesn't have a valid topic reference, skip adding it to the render context. | ||
continue | ||
} | ||
|
||
let dependencyReference = ResolvedTopicReference( | ||
bundleID: .init(rawValue: rawBundleID), | ||
path: url.path, | ||
fragment: url.fragment, | ||
// TopicRenderReference doesn't have language information. Also, the reference's languages _doesn't_ specify the languages of the linked entity. | ||
sourceLanguages: reference.sourceLanguages | ||
) | ||
topics[dependencyReference] = .init(renderReference: dependency, canonicalPath: nil, taskGroups: nil, source: nil, isDocumentationExtensionContent: false) | ||
} |
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'm not sure what this is fixing, chould we add this in a separate PR to reduce the amount of things this PR is changing?
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.
No. This change is necessary for this to work. It ensures that any topic references in the external entity's abstract is included in the linking page's references section.
If you want to see what this is for you can try commenting it out and see which tests fail.
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.
Adding a comment here with "This logic ensures that any topic references in the external entity's abstract are included in the linking page's references section" would be helpful for future maintainability
Tests/SwiftDocCTests/Infrastructure/TestExternalReferenceResolvers.swift
Show resolved
Hide resolved
Tests/SwiftDocCTests/LinkTargets/LinkDestinationSummaryTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/LinkTargets/LinkDestinationSummaryTests.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/External Data/OutOfProcessReferenceResolver.swift
Outdated
Show resolved
Hide resolved
@swift-ci please test |
005d1ca
to
cf1dfb4
Compare
@swift-ci please test |
cf1dfb4
to
2c6beba
Compare
@swift-ci please test |
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.
LGTM, thank you for addressing my comments
2c6beba
to
cced91e
Compare
@swift-ci please test |
…and external link resolver executables (swiftlang#1292)" This reverts commit 9413c59.
Bug/issue #, if applicable: rdar://159610082 #802 rdar://76656957&77906558&86334416&92254059&120424365 #341
Summary
This adds a new "version 2" of the communication protocol between DocC and external link resolver executables.
Some of the benefits of the new version:
One thing to note about the new version; because it encodes the abstract with the full fidelity and includes any references from the abstract, an externally curated page can display both transient links and images. Because of this, the warning about links or images in abstract is no longer needed (and is deprecated, to be removed (#341)).
Dependencies
None.
Testing
The easiest way to test this is by configuring DocC to call
bin/test-data-external-resolver
(updated to use the new version of the protocol) for external links and then add some external links with a "com.test.bundle" identifier in some minimal content.Note: the
test-data-external-resolver
need to be updated to reference an image in the abstract to verify that functionality.Note: external links on the page won't be clickable unless the archive is merged with an archive with that bundle identifier. To simulate this you can either create a separate archive and merge the two or you can add "com.test.bundle" in the only archive's
includedArchiveIdentifiers
list in its /index/index.json file.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded