Refactor can_stack_primitive_on_inputs
#2522
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2522 +/- ##
=======================================
Coverage 99.47% 99.47%
=======================================
Files 403 403
Lines 24197 24203 +6
=======================================
+ Hits 24070 24076 +6
Misses 127 127
|
gsheni
requested review from
dvreed77,
ozzieD,
rwedge,
tamargrey and
thehomebrewnerd
| @@ -1094,17 +1094,60 @@ def _find_root_primitive(feature): | |||
| return feature.primitive | |||
|
|
|||
|
|
|||
| def _check_if_stacking_is_prohibited( | |||
There was a problem hiding this comment.
Can you try to type this? might be hard, but would be helpful to add types to new things we create
There was a problem hiding this comment.
Added some type hints in 5178220
|
|
||
| if tup_primitive_stack_on is None or isinstance( | ||
| # we permit stacking only if it is not prohibited and meets the criterion to be permitted | ||
| if _check_if_stacking_is_permitted( |
There was a problem hiding this comment.
maybe I'm missing something, but prohibited and permitted should be mutually exclusive states, and so if its not prohibited, its permitted. Can you refactor even more to make this the case?
There was a problem hiding this comment.
Yeah I'm with Dave here, I like that you extracted this logic into separate functions but is it possible we could just merge these two functions together?
There was a problem hiding this comment.
@dvreed77 I don't think prohibited and permitted in this case are mutually exclusive. I think we can have cases where both checks are False or both are True because the logic checks are different between the "permitted" and "prohibited" functions. We want prohibited to take precedence, so in the case that both checks are True or both checks are False, we want the result to be "prohibited".
I might be missing something still, but I think those conditions make it a little tricker to refactor this further.
There was a problem hiding this comment.
I did add some additional comments to maybe clarify what is going on a little better. I'm open to other suggestions on how to improve further though.
There was a problem hiding this comment.
I agree with Nate here, I remember trying to find a way to clean it up before coming up with the original changes, but none were immediately apparent to me at the time. I think this PR was an initial attempt at refactoring the stacking logic at a surface level, mostly to play around with and discover the areas where we could refactor. A second, more fundamental refactor might be needed to simplify the deeper logic of how we handle stacking.
| continue | ||
|
|
||
| if f_primitive.base_of is None: | ||
| continue | ||
| if primitive_class in f_primitive.base_of: | ||
| continue | ||
|
|
||
| # if we reach this case, we default to stacking not being permitted | ||
| return False |
There was a problem hiding this comment.
since we continue into a return False here we could probably
just do: return _check_if_stacking_is_permitted()
There was a problem hiding this comment.
We only want to return when _check_if_stacking_is_permitted returns False, otherwise we need to keep going to make sure it's True for all inputs.
|
We should test this branch with the many features ecommerce test on looking glass to verify feature definitions are unchanged |
Tested this against |
can_stack_primitive_on_inputshelper function #2521