Skip to content
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

deprecate ExternalLocationReference in favor of DownloadReference #638

Merged

Conversation

QuietMisdreavus
Copy link
Contributor

Bug/issue #, if applicable: rdar://111121386

Summary

This PR refactors DownloadReference and ExternalLocationReference to deprecate the latter by incorporating it into the former. Now that there is a property on DownloadReference to prevent rewriting the URL upon encoding, this refactor takes advantage of that to incorporate the original uses of ExternalLocationReference back into DownloadReference.

Dependencies

None

Testing

This is primarily an internal refactor, but there are two major changes in the output: external-assets.json will no longer be generated (its previous contents will now be folded into the downloads property of assets.json, and references that would have used the externalLocation reference type (i.e. @CallToAction(url:_:)) now instead use the download reference type.

Steps:

  1. DOCC_JSON_PRETTYPRINT=YES swift run docc convert --emit-digest 'Tests/SwiftDocCTests/Test Bundles/SampleBundle.docc'
  2. Inspect the resulting built archive:
    a. The external-assets.json file should not exist.
    b. The assets.json file should contain a download reference with a url of "https://www.example.com/source-repository.git".
    c. The rendered JSON for MyExternalSample should contain a reference to "https://www.example.com/source-repository.git" with a reference type of download.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • [ n/a ] Added tests
  • Ran the ./bin/test script and it succeeded
  • [ n/a ] Updated documentation if necessary

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@@ -37,14 +37,12 @@ struct CodableRenderReference: Codable {
reference = try TopicRenderReference(from: decoder)
case .section:
reference = try TopicRenderReference(from: decoder)
case .download:
case .download, .externalLocation:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is important, but IIUC the line below won't work because DownloadReference(from: Decoder) re-decodes the type key?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my preference would be to leave this file as-is during the transition period. The downside is that we'll end up with a deprecation warning in the code base but it seems less risky overall.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, i edited this method because i didn't want to introduce a deprecation warning to our builds. I can put it back if that's what y'all would prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry – just thought of an additional thing.

Can we add a test that calls projectFiles() on a RenderNode that was created with @CallToAction(...download...)? Clients expect that calling that method will return the DownloadReference for any sample code pages.

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@ethan-kusters
Copy link
Contributor

@swift-ci please test

@QuietMisdreavus
Copy link
Contributor Author

@swift-ci Please test

@QuietMisdreavus QuietMisdreavus merged commit 2131c43 into swiftlang:main Jun 22, 2023
@QuietMisdreavus QuietMisdreavus deleted the unify-external-locations branch June 22, 2023 22:53
QuietMisdreavus added a commit to QuietMisdreavus/swift-docc that referenced this pull request Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants