-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Description
Hi there! I ran into this recently: both the sync and async Lock APIs have extend and do_extend typed as returning bool, but in practice only True is returned.
For example, here:
Lines 287 to 317 in b96d29c
| def extend(self, additional_time: Number, replace_ttl: bool = False) -> bool: | |
| """ | |
| Adds more time to an already acquired lock. | |
| ``additional_time`` can be specified as an integer or a float, both | |
| representing the number of seconds to add. | |
| ``replace_ttl`` if False (the default), add `additional_time` to | |
| the lock's existing ttl. If True, replace the lock's ttl with | |
| `additional_time`. | |
| """ | |
| if self.local.token is None: | |
| raise LockError("Cannot extend an unlocked lock", lock_name=self.name) | |
| if self.timeout is None: | |
| raise LockError("Cannot extend a lock with no timeout", lock_name=self.name) | |
| return self.do_extend(additional_time, replace_ttl) | |
| def do_extend(self, additional_time: Number, replace_ttl: bool) -> bool: | |
| additional_time = int(additional_time * 1000) | |
| if not bool( | |
| self.lua_extend( | |
| keys=[self.name], | |
| args=[self.local.token, additional_time, "1" if replace_ttl else "0"], | |
| client=self.redis, | |
| ) | |
| ): | |
| raise LockNotOwnedError( | |
| "Cannot extend a lock that's no longer owned", | |
| lock_name=self.name, | |
| ) | |
| return True |
Instead of False, these APIs use exceptions to communicate failure states.
The current return type can lead to developer confusion, e.g. developers mistakenly doing this:
if not lock.extend(...):
# handle lock extension failure...which doesn't work as expected, since lock failure causes a raise LockError instead.
Proposed solution
I think the simplest solution here would be to refine the return types on these APIs: instead of -> bool these APIs could be annotated with -> typing.Literal[True] to communicate clearly to developers that there's no meaningful conditional check on the return value.
Alternatives
An alternative to the proposed solution above would be to document in redis-py's docs that the return type of bool is really just the True value, and that users must handle errors solely through exception handling. I think this would be fine, but that it would be a lot clearer to have this encoded at the type layer additionally, per above 🙂