chore(typing): resolve time unit/zone set invariance

[dangotbanned]

What type of PR is this? (check all applicable)

• 💾 Refactor
• ✨ Feature
• 🐛 Bug Fix
• 🔧 Optimization
• 📝 Documentation
• ✅ Test
• 🐳 Other

Related issues

• Related PR (feat: add datetime selector #1822)

Checklist

• Code follows style guide (ruff)
• Tests added
• Documented the changes

If you have comments or can explain your changes, please do so below

[FBruzzesi]

Thanks @dangotbanned , I have a question relevant for 50% of the changes 🙈

[FBruzzesi]

Why wouldn't

Suggested change

	) -> tuple[set[TimeUnit], AbstractSet[str | None]]:
	) -> tuple[set[TimeUnit], set[str | None]]:

not work?

I though AbstractSet was deprecated

[dangotbanned]

Good question @FBruzzesi

It very much is deprecated, but until we up our minimum python to 3.9 we can't use this as a generic https://docs.python.org/3/library/collections.abc.html#collections.abc.Set

AbstractSet and Set both have a covariant TypeVar in the stubs https://github.com/python/typeshed/blob/cc8ca939c0477a49fcce0554fa1743bd5c656a11/stdlib/typing.pyi#L627

[FBruzzesi]

I feel like I am missing something, how come that for one variable we can use it and not for the other?

Even with a py3.8 venv I am getting no issues with pyright after annotating the return type and the respective variables to set[TimeUnit] and set[str | None] 🤔

[dangotbanned]

Even with a py3.8 venv I am getting no issues with pyright after annotating the return type and the respective variables to set[TimeUnit] and set[str | None] 🤔

Hmm, I'll need to take a look at this tomorrow.
IIRC, the difference was set[TimeUnit] can be used now because because it isn't a union.
But set[str | None] got expanded to set[str] | set[None] | set[str | None] - which was the issue

[dangotbanned]

AbstractSet[str | None] can match any of or all of those 3 (because of the variance)

[FBruzzesi]

Thanks for explaining! I am approving this in the meanwhile, but let me know if you manage to double check whenever you have the time 🙏🏼

[dangotbanned]

Thanks @FBruzzesi will do!

Absolutely need some time away from typing after #2007 😵

[dangotbanned]

Okay so I know what the issue was now @FBruzzesi

By using AbstractSet, we can safely drop the inline time_zones annotation.

Note

I forgot to do this though 🤦‍♂️

Image 1

We can also switch every annotation to set[str | None], as you discovered in #2012 (comment):

Image 2

But this requires annotating it like that one every usage.
Dropping the inline annotation shows the warning:

Image 3

Originally, I added a new alias for this - but later scrapped it since it only appears in these two functions:

_TimeZones: TypeAlias = AbstractSet[str | None]]

---

However, leaving the variables to inference only works for pyright sadly:

# mypy
narwhals/utils.py:1257: error: Argument 1 to <set> has incompatible type "str"; expected "None"  [arg-type]
narwhals/utils.py:1259: error: Set comprehension has incompatible type set[str | None]; expected set[str]  [misc]
narwhals/utils.py:1259: error: Set comprehension has incompatible type set[str | None]; expected set[None]  [misc]

I'm just pushing a little tweak that makes more sense to me

[dangotbanned]

See (b38403c)

[FBruzzesi]

Thanks @dangotbanned 🚀

[MarcoGorelli]

thanks all!