Remove the Confirmation.ExpectedCount marker protocol.
#689
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR replaces uses of `Confirmation.ExpectedCount` in our API surface with `RangeExpression<Int> & Sendable`. A composed protocol type `P<A> & Q` can only be expressed as `some`, not `any` as the latter is not yet implemented in the compiler (rdar://96960993), so we were using a separate protocol in place of that combination. The downside of doing so is that it makes the legibility of the interface worse. Xcode and other IDEs will offer you `confirmation(expectedCount: Confirmation.ExpectedCount)` but don't tell you that you need to write some range expression such as `0...2`. You have to dig into the protocol definition/documentation to figure it out. So this change changes the signature of the experimental `confirmation()` overload to take a `some RangeExpression<Int> & Sendable` and plumbs that through everywhere it's valid. Once we get to generating an `Issue`, we need an existential box (`any`) at which point we will just drop the `<Int>` bit until the compiler feature is added. We have a parameterized unit test that takes an array of these values, so to keep it building and passing, I moved a copy of the `ExpectedCount` protocol to the fixtures section of the test file containing that test; it's present only in our test target and not in our API surface.
grynspan
added
enhancement
grynspan
requested review from
stmontgomery,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@swift-ci test |
… how to handle .confirmationOutOfRange)
|
@swift-ci test |
stmontgomery
approved these changes
Sep 13, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces uses of
Confirmation.ExpectedCountin our API surface withRangeExpression<Int> & Sendable. A composed protocol typeP<A> & Qcan only be expressed assome, notanyas the latter is not yet implemented in the compiler (rdar://96960993), so we were using a separate protocol in place of that combination.The downside of doing so is that it makes the legibility of the interface worse. Xcode and other IDEs will offer you
confirmation(expectedCount: Confirmation.ExpectedCount)but don't tell you that you need to write some range expression such as0...2. You have to dig into the protocol definition/documentation to figure it out.So this change changes the signature of the experimental
confirmation()overload to take asome RangeExpression<Int> & Sendableand plumbs that through everywhere it's valid. Once we get to generating anIssue, we need an existential box (any) at which point we will just drop the<Int>bit until the compiler feature is added.We have a parameterized unit test that takes an array of these values, so to keep it building and passing, I moved a copy of the
ExpectedCountprotocol to the fixtures section of the test file containing that test; it's present only in our test target and not in our API surface.Checklist: