Refactor UnpackChain and add tests#5644
Refactor UnpackChain and add tests#5644chillenzer wants to merge 1 commit intoComputationalRadiationPhysics:devfrom
Conversation
23b4f0c to
b82f702
Compare
b82f702 to
ac2693c
Compare
| @@ -124,17 +124,6 @@ def alt(expr, alternative, *exprs, ignore=(AttributeError, TypeError, IndexError | |||
| return alternative | |||
There was a problem hiding this comment.
sorry, it isnt changed here but is this supposed to be return errors?
There was a problem hiding this comment.
Well, let's put it like this: It is the intended behaviour in the sense that I wanted to avoid the additional lambda: in cases where I just provide a fallback return value, e.g.,
# This is what I want to write
if alt(lambda: this_might_fail(x), False):
...But I thought I'd also occasionally want to write
alt(lambda: this_might_fail(x), lambda: this_never_fails(x))But I agree that this might lead to surprises, so that's probably just a bad design choice. Maybe a having an explicit kwarg to distinguish those cases is better:
# fallback is an expression:
alt(lambda: this_might_fail(x), e=lambda: this_never_fails(x))
# fallback is an instance:
alt(lambda: this_might_fail(x), i=False)The naming is not great but I want to keep it short because I want it to read just like a single intuitive expression (as opposed to the nested try: ... except: ... or if isinstance(...) blocks I'd have to write otherwise).
Naming suggestions:
- Callable:
c= "callable",e= "expression",f= "function - Instance:
i= "instance",o= "object",x= "a conventional name for a variable (opposed to a functionf)"
All of those could have an alt_ prepended, e.g., alt_e and alt_i for "alternative expression/instance".
Also there's a point to make that this_never_fails(x) could just be run in the main scope because, well, it never fails. So, maybe the expression fallback is just overengineering. (Of course, one could easily construct scenarios where this is beneficial but that doesn't mean they actually occur in our codebase.) I should check the codebase if I use it anywhere.
| NotImplemented, | ||
| ) | ||
| try: | ||
| return iter(self.obj) |
There was a problem hiding this comment.
will this return an iterator over strings? If so I suspect that's not what we want
There was a problem hiding this comment.
Oh, strings are so annoying in iterating contexts... -.-
| return iter([]) | ||
|
|
||
| if len(self._requests) == 1 or alt(lambda: hasattr(new_obj, self._requests[1]), False): | ||
| if len(self._requests) == 1 or alt(lambda: self._requests[1](new_obj), False): |
There was a problem hiding this comment.
this fails if the request on new_obj returns 0
|
Converting to draft because I want to rebase this on top of #5647 once that is merged. (Let alone the fact that there's important feedback.) |
|
Thanks for catching those! I was already wondering why I jumped through all the hoops in the first place. I'll write regression tests for those cases, design an intuitive behaviour for them and get back to you with the updates. |
This PR refactors
UnpackChaininto a cleaner implementation with more straightforward semantics and adds tests.