-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add ConditionTrait.evaluate() #909
base: main
Are you sure you want to change the base?
Conversation
I'm trying to decide what tests to add. Are existing tests enough, since I pretty much just moved existing logic to a different place? |
We aim for 100% code coverage when testing, so the new function should be tested (even if it is just a trivial "did this return true when I expected it to?" test.) |
|
||
|
||
/// Returns the result of evaluating the condition. | ||
public func evaluate() async throws -> Evaluation { |
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.
Perhaps just have this function return a tuple of (isEnabled: Bool, comment: Comment?)
.
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.
Do you have a strong preference? I like the enum better because with the tuple version the comment
field is meaningless when isEnabled
is 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.
I would prefer a tuple here.
The enum constrains us in the future if we decide we do want to emit some comment on success. (We haven't planned such a thing, but we could conceivably want that.) Adding an associated value to an enum case is a potentially source-breaking change.
Whereas if we changed what evaluate()
returns, we could do it by deprecating it and replacing it with a function that returns something else or has a different name or whatever.
|
||
|
||
/// Returns the result of evaluating the condition. | ||
public func evaluate() async throws -> Evaluation { |
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.
Separately: we need @_spi(Experimental)
here as this new API has not been through the formal review process.
/// The condition failed, potentially returning a `Comment`. | ||
case failure(Comment?) | ||
} | ||
/// The result of evaluating the condition. |
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 typealias is not necessary, but if you do feel strongly about including it, we'll want the tuple to include labels for both elements so we can document it fully. Something like:
/// The result of evaluating the condition.
///
/// - Parameters:
/// - wasMet: Whether or not the condition was met.
/// - comment: Optionally, a comment describing the result of evaluating the condition.
public typealias Evaluation = (_ wasMet: Bool, comment: Comment?)
(Feel free to tweak the wording—that was just off the top of my head.)
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.
I do like having it to avoid repeating the definition. I'll add the other label.
var commentOverride: Comment? | ||
|
||
|
||
/// Returns the result of evaluating the condition. |
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.
/// Returns the result of evaluating the condition. | |
/// Evaluate this instance's underlying condition. | |
/// | |
/// - Returns: The result of evaluating this instance's underlying condition. | |
/// |
You may also want to include a discussion paragraph after - Returns:
if there is additional information for the reader to consume. In particular, it may be worth noting that the result of this function is not cached.
@@ -19,6 +19,9 @@ | |||
/// - ``Trait/disabled(if:_:sourceLocation:)`` | |||
/// - ``Trait/disabled(_:sourceLocation:_:)`` | |||
public struct ConditionTrait: TestTrait, SuiteTrait { | |||
/// The result of evaluating the condition. | |||
public typealias Evaluation = (Bool, comment: Comment?) |
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 typealias will need @_spi(Experimental)
too
@@ -19,6 +19,9 @@ | |||
/// - ``Trait/disabled(if:_:sourceLocation:)`` | |||
/// - ``Trait/disabled(_:sourceLocation:_:)`` | |||
public struct ConditionTrait: TestTrait, SuiteTrait { | |||
/// The result of evaluating the condition. | |||
public typealias Evaluation = (Bool, comment: Comment?) |
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.
"EvaluationResult" may be more clear?
I plan to add some tests soon, at which point I think it will be appropriate to take the PR out of draft mode. |
The signature is `evaluate() async throws -> Evaluation`, where `Evaluation` is | ||
a `typealias` for the tuple already used for the callback in `Kind.conditional`, | ||
containing a boolean result and an optional comment. |
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.
Please include a full code block which shows the proposed additions, including the evaluate function, related typealias, and anything else public
, with the documentation comments. You can omit the implementation bodies of any functions, its only the interfaces that need to be shown.
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.
Added.
|
||
## Detailed design | ||
|
||
The signature is `evaluate() async throws -> Evaluation`, where `Evaluation` is |
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.
Looks like you did take the suggestion to rename the typealias to EvaluationResult
, so these references to the old name can be updated.
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.
Done.
Add
ConditionTrait.evaluate()
so that a condition can be evaluated independent of aTest
object.Motivation:
Currently, the only way a
ConditionTrait
is evaluated is inside theprepare(for:)
method. This makes it difficult and awkward for third-party libraries to utilize these traits because evaluating a condition would require creating a dummyTest
to pass to that method.Modifications:
Add
ConditionTrait.evaluate()
, andConditionTrait.Evaluation
enum for the return value.Result:
Public API allows for evaluating a
ConditionTrait
in any context.Checklist: