fix(redis): make xadd fields use SupportsItems

[sileht]

Using an InvariantMapping is wrong, the type of each values of the dict
can be different.

This change uses the SupportsItems Protocol instead to match as much as possible the
code.

[AlexWaygood]

Why?

[sileht]

Why?

You're too fast :) I didn't finish to write the description correctly.

That said, I just saw a discussion to drop types-redis, so not sure you continue to accept change to this typing.

(btw, redis 5.x builtin typing is far to be usable due to redis/redis-py#2933)

[srittau]

While I'm fine with replacing Mapping with an appropriate covariant protocol (we have a few of those available in _typeshed),we not should just revert this commit and replace the existing annotations with no annotations.

[sileht]

I propose another solution.

[srittau]

Hmm, redis-py seems to explicitly check for a dict:

https://github.com/redis/redis-py/blob/cc4bc1a544d1030aec1696baef2861064fa8dd1c/redis/commands/core.py#L3547

@sileht Can you give an example where type checkers have problems with the current annotations?

[AlexWaygood]

Hmm, redis-py seems to explicitly check for a dict:

FWIW, it does look to me like the current annotations we have are possibly the worst of both worlds. We don't get nice covariant types, since Mapping is invariant in its key types -- but we're also not particularly accurate, since redis-py does actually require a dict (for some reason) rather than just any old Mapping. So I think we should either change the stubs to be more accurate (Mapping -> dict), or change them to be more user-friendly (Mapping -> SupportsItems, or some other protocol). The status quo doesn't seem great.

I'd also be curious for @sileht's use case where the current annotations are causing a problem, though; it's hard to solve a problem if we don't know exactly what the problem is.

[sileht]

Currently, you can't write thing like:

class RedisStreamData(typing.TypedDict):
    foo: str
    bar: bytes

redis.xadd("stream", RedisStreamData({"foo": "bar", "bar": b"foo"}))

[srittau]

Considering @sileht's valid use case and the fact that we prefer false negatives over false positives, I think this change is the best we can do.

@sileht Could you add a comment before the fields argument, something like "Only accepts dict objects, but for variance reasons we use a looser annotation." (Maybe you can come up with a better wording.)

[JelleZijlstra]

Pyright is now unhappy because xadd doesn't have a return type. If I'm reading https://redis.io/commands/xadd/ right, it should return a str most of the time, but possibly None if nomkstream is True.

[JelleZijlstra]

That's the easier solution :)

[AlexWaygood]

Pyright is now unhappy because xadd doesn't have a return type. If I'm reading redis.io/commands/xadd right, it should return a str most of the time, but possibly None if nomkstream is True.

Oh I just ignored it for now. But if you have a better fix, feel free to push it :)

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉