[external-links] Display frameworks as beta only if all platforms are beta #938

anferbui · 2024-06-04T15:33:45Z

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

Summary

Symbols can have an availability per platform (iOS, macOS, etc), each of which can be marked as "beta" by providing some metadata when invoking docc:

swift run docc preview preview TestPackage.docc --platform name=macOS,version=40.0.0,beta=true --platform name=iOS,version=50.0.0,beta=true --platform name=watchOS,version=60.0.0,beta=true

A top-level badge is sometimes rendered for a symbol, depending on its platform availability.

Currently there is a mismatch in behaviour where:

externally resolved links will be shown as beta if any of its available platforms are in beta
https://github.com/apple/swift-docc/blob/8f1a9d206ed6cdcf3dccb091fa57216c6d546f21/Sources/SwiftDocC/Infrastructure/External%20Data/OutOfProcessReferenceResolver.swift#L158

internally resolved links will be shown as beta if all of its available platforms are in beta
https://github.com/apple/swift-docc/blob/8f1a9d206ed6cdcf3dccb091fa57216c6d546f21/Sources/SwiftDocC/Model/Rendering/DocumentationContentRenderer.swift#L208-L240

This behaviour is inconsistent and can be confusing for documentation writers. This PR proposes unifying the behaviour so that a symbol is shown as beta if all of its available platforms are in beta.

Implementation

Changes the behaviour of external link resolution in OutOfProcessReferenceResolver to only display a framework to be in beta if all the platforms its supported in are in beta.

This makes it match the behaviour for internal link resolution, so that the behaviour is consistent whether you're linking to a symbol internally within a framework or externally from another framework.

Also modifies LinkDestinationSummary to keep both of the external symbol resolution code paths in sync with each other. Hopefully in the future we can unify and have the same code path for both, but right now the logic is duplicated (and equal).

Dependencies

N/A.

Testing

You can set up a link resolver locally which has beta platform information in order to verify this fix:
link-resolver.zip

Excerpt:

"platforms": [
    {
      "name": "macOS",
      "introducedAt": "40.0.0",
      "beta": true
    },
    {
      "introducedAt": "50.0.0",
      "beta": true,
      "name": "iOS"
    },
    {
      "introducedAt": "60.0.0",
      "beta": false,
      "name": "watchOS"
    }
  ]

TestPackage.zip

Steps:

Download link-resolver and TestPackage.
Render documentation locally:

DOCC_LINK_RESOLVER_EXECUTABLE=link-resolver swift run docc preview TestPackage.docc --platform name=macOS,version=40.0.0,beta=true --platform name=iOS,version=50.0.0,beta=true

Verify that in http://localhost:8080/documentation/testpackage/betaclass, neither of the symbols mentioned have a top level badge that says the symbol is a beta symbol.

Checklist

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

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

Verification

Unit tests

Added unit tests for OutOfProcessReferenceResolver and ExternalPathHierarchyResolver since these changes affect both types of external link resolution.

Ran ./bin/test:

❯ ./bin/test
Building for debugging...
[122/122] Linking SwiftDocCPackageTests
Build complete! (7.75s)
[1685/1685] Testing SwiftDocCUtilitiesTests.DirectoryMonitorTests/testMonitorDoesNotTriggerUpdates
=> Checking for unacceptable language… okay.
=> Checking license headers… okay.
=> Validating scripts in bin subdirectory… okay.

Site rendering

Before	After

Documentation

As far as I can tell, no changes to the documentation are needed.

anferbui · 2024-06-05T16:01:26Z

@swift-ci please test

patshaughnessy

Looks good - just wondering if this is the right time for a larger refactoring to avoid duplicating more code like this. Maybe we merge this and then take on the larger work in another PR?

patshaughnessy · 2024-06-06T01:03:19Z

Sources/SwiftDocC/Infrastructure/Link Resolution/ExternalPathHierarchyResolver.swift

@@ -182,6 +182,15 @@ private extension Sequence<DeclarationRenderSection.Token> {
 // MARK: ExternalEntity

 private extension LinkDestinationSummary {
+    /// A value that indicates whether this symbol is under development and likely to change.


Looks like you're adding exactly the same code to both LinkDestinationSummary and OutOfProcessReferenceResolver.ResolvedInformation - would it be worth introducing a protocol?

Actually I just saw this comment:

public struct ResolvedInformation: Codable { // This type is duplicating the information from LinkDestinationSummary with some minor differences. // Changes generally need to be made in both places. It would be good to replace this with LinkDestinationSummary. // FIXME: https://github.com/apple/swift-docc/issues/802

Is this a good time to take on this refactoring work?

Yes that comment was exactly my reasoning for duplicating the API as closely as possible, thanks for calling that out!

I'll look into refactoring the code to see how much of a code change it would be to unify the code paths.

The change to unify the code paths appears to be quite involved, I'll look into it further tomorrow but I think we should tackle that as a separate PR regardless.

That's definitely for a separate PR. It's would both be quite involved and a bit of a risky change.
#802

Tests/SwiftDocCUtilitiesTests/OutOfProcessReferenceResolverTests.swift

Tests/SwiftDocCTests/Test Bundles/AvailabilityBetaBundle.docc/Info.plist

Tests/SwiftDocCTests/XCTestCase+LoadingTestData.swift

Tests/SwiftDocCUtilitiesTests/OutOfProcessReferenceResolverTests.swift

Changes the behaviour of external link resolution in `OutOfProcessReferenceResolver` to only display a framework to be in beta if all the platforms its supported in are in beta. This makes it match the behaviour for internal link resolution, so that the behaviour is consistent whether you're linking to a symbol internally within a framework or externally from another framework. Resolves rdar://128997995.

Makes changes to `OutOfProcessReferenceResolver .ResolvedInformation` so that it can be replaced by `LinkDestinationSummary` at some point in the future. Both `OutOfProcessReferenceResolver .ResolvedInformation` and `LinkDestinationSummary` now contain information about whether the resolved external link points to a symbol in beta or not, using equivalent API. This is an attempt at keeping both of the logics about whether an external symbol is in beta in sync with each other. Hopefully in the future we can unify and have the same code path for both, but right now the logic is duplicated (and equal). Resolves rdar://128997995.

Adds a unit test which verifies that the logic for determining whether a symbol is in beta when using `ExternalPathHierarchyResolver` is as expected. Resolves rdar://128997995.

Test file `OutOfProcessReferenceResolverTests` had inconsistent and confusing formatting, particularly with how the type of `makeResolver` was being defined across multiple lines in a way inconsistent with the rest of the codebase. `makeResolver` is now declared as a one-liner and fixes other minor formatting/spacing issues.

The file isn't needed, so removing it.

We want to avoid spreading the configure context parameter to the other test helpers, as we're slowly working to make the context not be configured after creation. Instead, `ExternalPathHierarchyResolverTests` now calls the underlying `loadBundle(from:configureContext:)` directly,

anferbui · 2024-06-12T16:22:05Z

@swift-ci please test

d-ronnqvist

This looks great. Thank you

… beta (swiftlang#938) * Display frameworks as beta only if all platforms are beta Changes the behaviour of external link resolution in `OutOfProcessReferenceResolver` to only display a framework to be in beta if all the platforms its supported in are in beta. This makes it match the behaviour for internal link resolution, so that the behaviour is consistent whether you're linking to a symbol internally within a framework or externally from another framework. * Refactor code to have same behaviour in both external link resolvers Makes changes to `OutOfProcessReferenceResolver .ResolvedInformation` so that it can be replaced by `LinkDestinationSummary` at some point in the future. Both `OutOfProcessReferenceResolver .ResolvedInformation` and `LinkDestinationSummary` now contain information about whether the resolved external link points to a symbol in beta or not, using equivalent API. This is an attempt at keeping both of the logics about whether an external symbol is in beta in sync with each other. Hopefully in the future we can unify and have the same code path for both, but right now the logic is duplicated (and equal). * Add a test for beta logic in ExternalPathHierarchyResolver Adds a unit test which verifies that the logic for determining whether a symbol is in beta when using `ExternalPathHierarchyResolver` is as expected. * Fix formatting Test file `OutOfProcessReferenceResolverTests` had inconsistent and confusing formatting, particularly with how the type of `makeResolver` was being defined across multiple lines in a way inconsistent with the rest of the codebase. `makeResolver` is now declared as a one-liner and fixes other minor formatting/spacing issues. * Remove noop Info.plist The file isn't needed, so removing it. * Remove changes to testBundleAndContext helpers We want to avoid spreading the configure context parameter to the other test helpers, as we're slowly working to make the context not be configured after creation. Instead, `ExternalPathHierarchyResolverTests` now calls the underlying `loadBundle(from:configureContext:)` directly. --- Resolves rdar://128997995.

anferbui force-pushed the tweak-beta-resolution branch from a50e773 to 0cab664 Compare June 5, 2024 15:59

anferbui marked this pull request as ready for review June 5, 2024 16:01

anferbui requested a review from d-ronnqvist June 5, 2024 16:01

patshaughnessy reviewed Jun 6, 2024

View reviewed changes

d-ronnqvist reviewed Jun 11, 2024

View reviewed changes

anferbui added 3 commits June 12, 2024 17:01

Add a test for beta logic in ExternalPathHierarchyResolver

d25a051

Adds a unit test which verifies that the logic for determining whether a symbol is in beta when using `ExternalPathHierarchyResolver` is as expected. Resolves rdar://128997995.

anferbui force-pushed the tweak-beta-resolution branch from 0cab664 to d25a051 Compare June 12, 2024 16:01

anferbui added 3 commits June 12, 2024 17:06

Remove noop Info.plist

041cd80

The file isn't needed, so removing it.

d-ronnqvist approved these changes Jun 12, 2024

View reviewed changes

anferbui merged commit b46c7b5 into swiftlang:main Jun 13, 2024
2 checks passed

anferbui deleted the tweak-beta-resolution branch June 13, 2024 09:51

anferbui mentioned this pull request Jun 13, 2024

[6.0] [external-links] Display frameworks as beta only if all platforms are beta #952

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[external-links] Display frameworks as beta only if all platforms are beta #938

[external-links] Display frameworks as beta only if all platforms are beta #938

anferbui commented Jun 4, 2024 •

edited

Loading

anferbui commented Jun 5, 2024

patshaughnessy left a comment

patshaughnessy Jun 6, 2024

anferbui Jun 6, 2024

anferbui Jun 6, 2024

d-ronnqvist Jun 10, 2024 •

edited

Loading

anferbui commented Jun 12, 2024

d-ronnqvist left a comment

[external-links] Display frameworks as beta only if all platforms are beta #938

[external-links] Display frameworks as beta only if all platforms are beta #938

Conversation

anferbui commented Jun 4, 2024 • edited Loading

Summary

Implementation

Dependencies

Testing

Checklist

Verification

anferbui commented Jun 5, 2024

patshaughnessy left a comment

Choose a reason for hiding this comment

patshaughnessy Jun 6, 2024

Choose a reason for hiding this comment

anferbui Jun 6, 2024

Choose a reason for hiding this comment

anferbui Jun 6, 2024

Choose a reason for hiding this comment

d-ronnqvist Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

anferbui commented Jun 12, 2024

d-ronnqvist left a comment

Choose a reason for hiding this comment

anferbui commented Jun 4, 2024 •

edited

Loading

d-ronnqvist Jun 10, 2024 •

edited

Loading