feat: Allow allOf between enums, string, or int properties that are subtypes
#423
Conversation
forest-benchling
changed the title
allOf enum and string/intallOf between enums, string, int that are subtypes
forest-benchling
changed the title
allOf between enums, string, int that are subtypesallOf between enums, string, or int properties that are subtypes
Codecov Report
@@ Coverage Diff @@
## main #423 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 47 47
Lines 1563 1579 +16
=========================================
+ Hits 1563 1579 +16
Continue to review full report at Codecov.
|
|
Hi @dbanty @emann, it looks like this PR as well as #421 have been open for over 2 months now. I just wanted to check in and ask what your bandwidth looks like, and when you might be able to get around to reviewing it? On our side, we're trying to decide whether to convert to using the latest upstream, or continue maintaining our own fork. I know you've mentioned that it's now an option for non-Triax folks to help maintain the repo, so that's another potential option. Let me know what you think. |
|
Hey @forest-benchling, I should have some time this weekend to work on open source stuff so I’ll make sure to prioritize this one. I still feel really weird about #421 since the ask is basically to add runtime type checking that doesn’t make sense given the static types. The right move would be to change the static types. I’m hoping that https://www.python.org/dev/peps/pep-0661/ leads to improved I’ll address #421 this weekend to, just not totally sure at the moment what way will be most palatable |
| return any( | ||
| [ | ||
| _is_string_enum(first) and isinstance(second, StringProperty), | ||
| _is_int_enum(first) and isinstance(second, IntProperty), | ||
| _is_string_enum(first) and _is_string_enum(second) | ||
| # cast because MyPy fails to deduce type | ||
| and values_are_subset(cast(EnumProperty, first), cast(EnumProperty, second)), | ||
| _is_int_enum(first) and _is_int_enum(second) | ||
| # cast because MyPy fails to deduce type | ||
| and values_are_subset(cast(EnumProperty, first), cast(EnumProperty, second)), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Using any like this will cause every single case to be evaluated before any is, causing extra work. Here I think the best bet is to refactor into an if/return sequence.
There was a problem hiding this comment.
Good point. Thanks @dbanty for taking care of this.
…pes [#379, #423, #461]. Thanks @forest-benchling! (#461) Co-authored-by: Forest Tong <forest@benchling.com>
Closes #379. Does not handle the case of two enums where neither one is a subset of the other, where technically you might want to take the intersection of the two.