Typing: `Lock.extend` and `Lock.do_extend` could return `Literal[True]`

[woodruffw]

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:

redis-py/redis/lock.py

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 🙂

[zanieb]

It might even be better to just return None since the True has no semantic meaning, though that's probably breaking.

[petyaslavova]

Hi @zanieb, thank you for reporting this problem! We will have a look at it.