jd
Type checking in operators
The current implementation of wait_base.__add__ and wait_base.__radd__ do almost no type checking. Thus it is possible to create nonsensical wait conditions.
For instance, I accidentally wrote this code which is accepted:
bad_waiter = tenacity.wait_none + tenacity.wait_random(1, 2)
but using this wait condition will result in errors:
TypeError: __init__() got an unexpected keyword argument 'retry_state'
Confusingly, as stop_never is an object (of class _stop_never), this is correct:
good_stopper = tenacity.stop_never | tenacity.stop_after_attempt(2)
IMHO, wait_base.__add__ and wait_base.__radd__ should check that both arguments are wait_base. Given the peculiar nature of those operators in Tenacity, I can't see problems. Similarly operators in stop_base should check that both arguments are stop_base.
Please note that:
- the modification is rather easy
- I don't think this would raise compatibility issues (since any working code is not affected)
Maybe classes that combine wait conditions (wait_combine and wait_chain) or stop conditions (stop_any and stop_all) should perform such checks.
To give a bit of context, I was working on integrating tenacity in the BioMAJ software. The goal is to be able to configure BioMAJ with text files containing thinks like wait_none() + wait_random(1, 2). To do so I wrote a parser of wait and stop strategies based on Simple Eval: functions are the stop/wait classes constructors and operators are their tenacity operators. This worked well (kudos to those 2 libraries) except that I got confused between wait_none (which is a class) and stop_never (which is an object) and started to see weird results in some expressions that are accepted but make no sense. Modifying operators would raise error when parsing (which is expected).
Doing type checking goes against duck typing, so this is usually frowned upon. I'm sorry you made a mistake when writing the code.
I think wait_none should be an object and not a class, that's been a mistake. It's hard to fix now, maybe it should just be removed anyhow.
I agree this goes against duck typing but in that case I hardly see how those operations could be extended. Also, I forgot to mention that wait_combine(wait_random(1, 2), 4) works too.
Maybe there is a simple way to check that the wait condition (or stop condition) is correct so I can add that after parsing the string to raise the error early.
It's your call, though.
I was thinking about a middleground: we could type check the argument of wait_combine and remove __radd__ (and make __add__ return NotImplemented for other classes). This way other modules can define their own additions (which is the purpose of __radd__) and we can avoid most errors in tenacity. This would break the use of sum to add wait conditions however (except if we use wait_none as first value).
PR #212 has been refused (closed without merge) so I think that this can be closed too.