Fix & normalize typing for chunks #8247
Conversation
I noticed that `"auto"` wasn't allowed as a value in a dict. So this normalizes all chunk types, and defines the mapping as containing the inner type. Allows removing some ignores (though also adds one). One question — not necessary to answer now — is whether we should allow a tuple of definitions, for each dimension. Generally we use names, which helps prevent mistakes, and allows us to be less concerned about dimension ordering.
| if isinstance(chunks, (Number, str, int)): | ||
| chunks = dict.fromkeys(self.dims, chunks) | ||
| if not isinstance(chunks, Mapping): | ||
| # We need to ignore since mypy doesn't recognize this can't be `None` |
There was a problem hiding this comment.
Wait... is the above check for None correct? I'm not sure that the chunks_kwargs can be None, isn't it always a dict? So this would make the warning not reachable...
There was a problem hiding this comment.
I don't think chunks_kwargs can be None, but chunks can... Does that answer your Q?
There was a problem hiding this comment.
Well, it answers the question.
But then this means that the above code is wrong because the if statement is never reachable...
Doesn't need to be fixed in this PR, but maybe rewriting this will remove the need for the type ignore.
There was a problem hiding this comment.
Oh I see what you mean...
| T_Chunks = Union[int, dict[Any, Any], Literal["auto"], None] | ||
| # int, Literal["auto"], None, tuple[int, ...], tuple[tuple[int, ...], ...] | ||
| # In some cases we don't allow `None`, which this doesn't take account of. | ||
| T_ChunkDim: TypeAlias = Union[int, Literal["auto"], None, tuple[int, ...]] |
There was a problem hiding this comment.
Slowly we should agree on a naming convention, haha.
But the addition of TypeAlias is nice, we should do that for all other types here as well.
There was a problem hiding this comment.
Ha! I even went back & forth on whether this should be T_ given it's not a TypeVar or Generic... But then I saw it existed elsewhere and didn't want to make a big change without having a strong view...
Based on comment in pydata#8247. This doesn't make it perfect, but allows the warning to get hit and clarifies the type comment, as a stop-gap
Based on comment in pydata#8247. This doesn't make it perfect, but allows the warning to get hit and clarifies the type comment, as a stop-gap
* Refine `chunks=None` handling Based on comment in #8247. This doesn't make it perfect, but allows the warning to get hit and clarifies the type comment, as a stop-gap * Test avoiding redefinition --------- Co-authored-by: Illviljan <[email protected]>
I noticed that
"auto"wasn't allowed as a value in a dict. So this normalizes all chunk types, and defines the mapping as containing the inner type.Allows removing some ignores (though also adds one).
One question — not necessary to answer now — is whether we should allow a tuple of definitions, for each dimension. Generally we use names, which helps prevent mistakes, and allows us to be less concerned about dimension ordering.