Add BooleanArtifact
#905
Add BooleanArtifact
#905
Conversation
| return BooleanArtifact(True) | ||
| elif value.lower() == "false": | ||
| return BooleanArtifact(False) | ||
| raise ValueError(f"Cannot convert string literal '{value}' to BooleanArtifact") |
There was a problem hiding this comment.
should this return None or raise an exception?
There was a problem hiding this comment.
I vote for raise an exception.
If the user's code assumes that the parse always succeeds and then it fails, returning None will (most likely) cause an exception to be raised by code that consumes this output. Then it will be the user's responsibility to trace it back to here, so we might as well do it for them.
|
|
||
| @define | ||
| class BooleanArtifact(BaseArtifact): | ||
| value: bool = field(converter=bool, metadata={"serializable": True}) |
There was a problem hiding this comment.
passing in a value on init will get coerced into a bool, whereas using parse_bool will check for the literal true or false. should that check be done on init instead?
There was a problem hiding this comment.
I think having these two methods of creating a BooleanArtifact seems reasonable. The default (init) way being a little more flexible, but still offering a stricter option for strings specifically.
If you don't offer the bool coercion option and the use case comes up, then what would that look like? like this: BooleanArtifact(bool(x))? I suppose that's easy enough, but BooleanArtifact(x) is a little better. Actually, the parse_bool wouldn't work for a literal True or False, so I definitely vote for keeping both creation options.
There was a problem hiding this comment.
the init method will always convert the input to a boolean. this can lead to suprising behavior sometimes where BooleanArtifact("False").value is True. parse_bool is to parse string literal values as a convenience. thinking about it now, seems more "pythonic" to keep the init as to whatever python interprets as a bool, and use parse_bool to parse string literal values as a convenience
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
| return BooleanArtifact(True) | ||
| elif value.lower() == "false": | ||
| return BooleanArtifact(False) | ||
| raise ValueError(f"Cannot convert string literal '{value}' to BooleanArtifact") |
There was a problem hiding this comment.
I vote for raise an exception.
If the user's code assumes that the parse always succeeds and then it fails, returning None will (most likely) cause an exception to be raised by code that consumes this output. Then it will be the user's responsibility to trace it back to here, so we might as well do it for them.
|
|
||
| @define | ||
| class BooleanArtifact(BaseArtifact): | ||
| value: bool = field(converter=bool, metadata={"serializable": True}) |
There was a problem hiding this comment.
I think having these two methods of creating a BooleanArtifact seems reasonable. The default (init) way being a little more flexible, but still offering a stricter option for strings specifically.
If you don't offer the bool coercion option and the use case comes up, then what would that look like? like this: BooleanArtifact(bool(x))? I suppose that's easy enough, but BooleanArtifact(x) is a little better. Actually, the parse_bool wouldn't work for a literal True or False, so I definitely vote for keeping both creation options.
vachillo
force-pushed
the
boolean
branch
4 times, most recently
from
484670c
to
daf0783
Compare
artifact that is explicitly
TrueorFalse.i want to output a boolean from a
Taskand do something with that information later, and checking the string value of aTextArtifactis clunky.incremental update to support #904