Skip to content

Conversation

@sofiaromorales
Copy link
Contributor

@sofiaromorales sofiaromorales commented Oct 13, 2024

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

Summary

A node can only be declared as beta if it contains at least one availability item that includes all the defined characteristics. This code introduces a precondition requiring the node to have at least one availability item to be considered as potentially in beta.

Before this change, a node would be considered as beta when its availability set was empty.

Dependencies

N/A

Testing

Review unit tests.

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
  • Updated documentation if necessary

For a node to be declared as beta it has to contain at least a single availability item that contains all the already defined characteristics.

This code adds a precondition where at least the node needs to have one availability item to be considered as potentially in beta.

Before this change, if at somewhere in the code, the availability items got removed from the node, the node would be considered as beta.

rdar://137762221
@sofiaromorales sofiaromorales marked this pull request as ready for review October 13, 2024 17:08
])
let reference = ResolvedTopicReference(bundleIdentifier: bundle.identifier, path: "/documentation/MyKit/MyClass", sourceLanguage: .swift)
let node = try context.entity(with: reference)
(node.semantic as? Symbol)?.availability = SymbolGraph.Symbol.Availability(availability: [])
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a blocking comment but something to keep in mind.

Modifying a page after the context has fully registered the data provider assumes that there are no derivatives of the original value elsewhere that was created while the context registered the data provider.

This means that the test could be exercising a different behavior than a real build would and could pass even though a real build would have a bug.

I think that there are no derivates of the availability information today, but there could be in the future in which case this test could hide a bug because it passes and there's no other test that exercises the real-build behavior.

Because of this, it's preferred to modify the inputs before they're registered with the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true that data could be modified after the fact, but although this generally shouldn't be done, having this precondition adds an extra layer of security to the code.

@sofiaromorales
Copy link
Contributor Author

@swift-ci please test

@sofiaromorales
Copy link
Contributor Author

@swift-ci please test

@sofiaromorales sofiaromorales merged commit 6b4d5a3 into swiftlang:main Oct 14, 2024
2 checks passed
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.

2 participants