-
Notifications
You must be signed in to change notification settings - Fork 147
Support beta status in navigator #1249
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
base: main
Are you sure you want to change the base?
Conversation
The `isBeta` attribute is part of `TopicRenderReference` [1], and is already computed based on the platforms [2][3] (though the platform information itself is dropped). This changes propagates the property to `ExternalRenderNode` and `ExternalRenderNodeMetadataRepresentation` so that we can ultimately store it as part of the navigator `RenderIndex`. [4] Fixes rdar://155521394. [1]: https://github.com/swiftlang/swift-docc/blob/f968935b770b0011d7aa28f59eda22a8407282b7/Sources/SwiftDocC/Model/Rendering/References/TopicRenderReference.swift#L82-L85 [2]: https://github.com/swiftlang/swift-docc/blob/f968935b770b0011d7aa28f59eda22a8407282b7/Sources/SwiftDocC/Infrastructure/External%20Data/OutOfProcessReferenceResolver.swift#L158 [3]: https://github.com/swiftlang/swift-docc/blob/f968935b770b0011d7aa28f59eda22a8407282b7/Sources/SwiftDocC/Infrastructure/External%20Data/OutOfProcessReferenceResolver.swift#L592-L598 [4]: https://github.com/swiftlang/swift-docc/blob/f968935b770b0011d7aa28f59eda22a8407282b7/Sources/SwiftDocC/Indexing/RenderIndexJSON/RenderIndex.swift#L251
Adds a computed property to `NavigatorIndexableRenderMetadataRepresentation` which derives whether the navigator item `isBeta` or not. This uses the same logic used in other places in the codebase [1]. Fixes rdar://155521394. [1]: https://github.com/swiftlang/swift-docc/blob/f968935b770b0011d7aa28f59eda22a8407282b7/Sources/SwiftDocC/Infrastructure/External%20Data/OutOfProcessReferenceResolver.swift#L592-L598
Propagates the `isBeta` property to the `NavigatorItem` type from `NavigatorIndexableRenderMetadataRepresentation`. When we index a new node, whether the item is beta or not will now be captured as part of the navigator This is preparatory work before this property is propagated to the `RenderIndex` [1]. Fixes rdar://155521394. [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/RenderIndexJSON/RenderIndex.swift#L257-L259
The isBeta and isExternal properties weren't being serialised as part of `NavigatorItem`. These properties are used to initialise to `RenderIndex.Node` during the conversion to `index.json` [1] and must be preserved when navigator indexes are written to disk [2] so that when they are read [3], we don't drop beta and external information. This serialisation happens during the `finalize` step [4] and the deserialisation can be invoked via `NavigatorIndex.readNavigatorIndex` [5]. Otherwise, the values can be lost on serialisation roundtrip. [1]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/RenderIndexJSON/RenderIndex.swift#L329 [2]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/Navigator/NavigatorTree.swift#L195 [3]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/Navigator/NavigatorTree.swift#L266 [4]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift#L1193 [5]: https://github.com/swiftlang/swift-docc/blob/65aaf926ec079ddbd40f29540d4180a70af99e5e/Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift#L157
All that was left was that on initialisation of a `RenderIndex.Node`, we propagate the `isBeta` value from the `NavigatorItem`. With this change, beta information is now available in the navigator, encoded as `"beta": true` in the resulting `index.json` file. Fixes rdar://155521394.
Update public documentation to add missing parameters
@swift-ci please test |
@swift-ci please test |
1 similar comment
@swift-ci please test |
var isBeta: Bool { | ||
guard let platforms, !platforms.isEmpty else { | ||
return false | ||
} | ||
|
||
return platforms.allSatisfy { $0.isBeta == true } | ||
} |
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 code it's repeated in LinkDestinationSummary
ResolvedInformation
and now here. Might be better to unify these so it does not become a maintenance nightmare
public static func == (lhs: NavigatorItem, rhs: NavigatorItem) -> Bool { | ||
return lhs.pageType == rhs.pageType && | ||
lhs.languageID == rhs.languageID && | ||
lhs.title == rhs.title && | ||
lhs.platformMask == rhs.platformMask && | ||
lhs.availabilityID == rhs.availabilityID && | ||
lhs.isBeta == rhs.isBeta && | ||
lhs.isExternal == rhs.isExternal | ||
} | ||
|
||
// MARK: - Hashable | ||
// Needed because a Swift class's synthesized Hashable conformance doesn't take into account properties which have default values as part of the designated initializer. | ||
|
||
public func hash(into hasher: inout Hasher) { | ||
hasher.combine(pageType) | ||
hasher.combine(languageID) | ||
hasher.combine(title) | ||
hasher.combine(platformMask) | ||
hasher.combine(availabilityID) | ||
hasher.combine(isBeta) | ||
hasher.combine(isExternal) | ||
} |
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.
Is this true? If so, that feels worthy of a bug on the compiler that we should reference in these comments.
|
||
length = MemoryLayout<UInt8>.stride | ||
// To ensure backwards compatibility, handle both when `isBeta` has been encoded and when it hasn't | ||
if cursor < data.count { |
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.
Should this check that cursor + length
fits within the data? Currently, it's still possible that this code would index out of bounds. (same below)
} | ||
|
||
// To ensure backwards compatibility, handle both when `isExternal` has been encoded and when it hasn't | ||
if cursor < data.count { |
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.
Can these two ever be read/written individually or should they be checked together inside a single if-statement?
If they can be read/written individually, how is it possible to know which boolean a single value represents?
XCTAssertEqual(item, fromData) | ||
} | ||
|
||
func testNavigatorItemRawDumpBackwardCompatibility() { |
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.
Nice test 👍
minor: Some of the modified files should also update their license comments. After merging main you can use the utility in bin/update-license-comments/ to help with that. |
Bug/issue #, if applicable: rdar://155521394
Summary
Currently DocC has some code in
RenderIndex
to be able to store whether a navigation item is beta or not, but the feature isn't supported yet:swift-docc/Sources/SwiftDocC/Indexing/RenderIndexJSON/RenderIndex.swift
Lines 257 to 259 in 65aaf92
This PR propagates beta information for both render nodes and external render nodes to the navigation index such that we can ultimately encode the information as part of the navigator (i.e.
index.json
file):Same as in other parts of the codebase, an item is considered to be in beta if all of its platforms are in beta.
Navigator comparison (using swift-docc-render):
Dependencies
N/A
Testing
Previewed a custom bundle locally which contained both an external and local link which were both beta.
index.json
looked as expectedSteps:
/path/to/swift-docc/bin/test-data-external-resolver
and modifying the data as desiredDOCC_LINK_RESOLVER_EXECUTABLE=/path/to/swift-docc/bin/test-data-external-resolver swift run docc preview --platform name=macOS,version=1.0.0,beta=true --platform name=watchOS,version=2.0.0,beta=true --platform name=tvOS,version=3.0.0,beta=true --platform name=iOS,version=4.0.0,beta=true --platform "name=Mac Catalyst,version=4.0.0,beta=true" --platform name=iPadOS,version=4.0.0,beta=true Example.docc
index.json
is showing the external reference as a node, and that the node has a propertybeta=true
:Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded