Conversation
| default, | ||
| subject_attributes, | ||
| VariationType.NUMERIC, | ||
| # convert to float in case we get an int |
There was a problem hiding this comment.
👍 Took me a moment to understand – I first thought you were going to rescue a mismatch (getNumeric for an Integer flag) which I didn't like. But what you're doing is ensuring that 3.0 (a valid variation for a numeric flag) is indeed returned as a float and not as the integer 3.
| # we can convert int to float | ||
| return isinstance(value, float) or isinstance(value, int) |
There was a problem hiding this comment.
or isinstance(value, int) seems unnecessary? With this PR we always convert to float? Other than in one of the tests where I see that you explicitly rely on this behavior. In any case this is fine to keep - it's more that I want to check my understanding.
There was a problem hiding this comment.
Without the or this check would fail for an integer value such as 1, which we can convert to a float
There was a problem hiding this comment.
I get it now. The cast to float is done after validating the value type, so this is indeed necessary.
| if condition.value: | ||
| return subject_value is None | ||
| return subject_value is not None |
There was a problem hiding this comment.
supernit, optional: could write as return (subject_value is None) if condition.value else (subject_value is not None)
2 participants
labels: mergeable
Fixes: #issue
Motivation and Context
Description
How has this been tested?