Clean up Dims type annotation #8606
Conversation
xarray/core/types.py
Outdated
| @@ -182,8 +182,7 @@ def copy( | |||
| DsCompatible = Union["Dataset", "DaCompatible"] | |||
| GroupByCompatible = Union["Dataset", "DataArray"] | |||
|
|
|||
| Dims = Union[str, Iterable[Hashable], "ellipsis", None] | |||
| OrderedDims = Union[str, Sequence[Union[Hashable, "ellipsis"]], "ellipsis", None] | |||
| Dims = Union[Hashable, Sequence[Hashable], None] | |||
There was a problem hiding this comment.
ellipsis is a subclass of Hashable
There was a problem hiding this comment.
Am on phone but this has been the subject of much discussion, worth reviewing that...
There was a problem hiding this comment.
So there are good reasons to have str | Iterable[Hashable] rather than Hashable | Iterable[Hashable]...
Is there a cost of leaving ellipsis in? It's nice to have it be explicit, even if it doesn't affect the type resolution?
(+1 to much of the PR though, updating definitions to use Dims...)
crusaderky
changed the title
|
Ready for review |
| @@ -297,7 +298,7 @@ def test_parse_dims_replace_none(dim: None | ellipsis) -> None: | |||
| pytest.param(["x", 2], id="list_missing_all"), | |||
| ], | |||
| ) | |||
| def test_parse_dims_raises(dim: str | Iterable[Hashable]) -> None: | |||
| def test_parse_dims_raises(dim): | |||
There was a problem hiding this comment.
Why remove these annotations? IIUC these get mypy to use the tests to test our annotations?
There was a problem hiding this comment.
The only thing that they validate is that the signature on the test matches the signature declared for the function being tested - quite pointless IMHO. Notably, they don't validate the parameters being passed from the lines above.
There was a problem hiding this comment.
the signature on the test matches the signature declared for the function being tested - quite pointless IMHO
I'm not completely sure what you mean by this. But without check_untyped_defs, having the -> None is the only way we test whether our annotations are correct (or am I wrong there?)
I think you should revert removing the annotations, at least the -> None
There was a problem hiding this comment.
What I was trying to say is that the signature in the unit test causes mypy to verify that the signature declared in the unit test, dim: str | Iterable[Hashable] is indeed compatible with the signature declared in the parse_dims function, dim: Dims. Which IMHO is pointless.
It would have been useful if it verified that the values with which dim is actually populated in the test are legal for the Dims type, but it does not do that.
For example, in the future someone may short-sightedly change the definition of Dims from str | Collection[Hashable] to str | Sequence[Hashable]. One of the parameters in the test is
pytest.param({"a", 1}, tuple({"a", 1}), id="non_sequence_collection"),Nothing will trip, with or without annotations in the test signature, unless someone changes the actual implementation of parse_dims to crash if you pass a set to it.
Annotating the test signature would become useful if we rewrote it without parametrization:
def test_parse_dims() -> None:
all_dims = ("a", "b", 1, ("b", "c")) # selection of different Hashables
# non-sequence collection
actual = utils.parse_dims({"a", 1}, all_dims, replace_none=False)
assert actual == tuple({"a", 1})
... # repeat for all other use caseswhich would be a valid choice, but it would fail on the first bad use case instead of moving on and it would be less immediately obvious what broke. You win some, you lose some.
Would you like me to open a new PR where I rewrite the unit test without parametrization and with annotations?
There was a problem hiding this comment.
What I was trying to say is that the signature in the unit test causes mypy to verify that the signature declared in the unit test,
dim: str | Iterable[Hashable]is indeed compatible with the signature declared in the parse_dims function,dim: Dims. Which IMHO is pointless.
Totally agree!
Annotating the test signature would become useful if we rewrote it without parametrization:
Yes. In this specific case it's still slightly useful to have annotations on this function.
- The
dimtyping check isn't that useful, because we supply it atm all_dimswill be checked
Would you like me to open a new PR where I rewrite the unit test without parametrization and with annotations?
Sorry, no. (and to the extent you're interpreting my comment as arguably bad suggestions, I apologize, I wasn't suggesting doing this)
The thing that I do think we should do is get to a point where tests checking as many annotations as possible. There are two ways to do this:
check_untyped_defs=True-> Noneon test functions
So even though in this case it's only slightly useful:
- There's no downside
- The principle of having
-> Noneis helpful, and gets us closer to having a blanketcheck_typed_defs
...so I think we should restore -> None (and nothing else)
Just to put this in perspective, I'm not trying to be difficult / antagonistic. We previously had lots of incorrect annotations! And so I have done a decent amount of work moving xarray on this — you can see the progress we've made at converting the library to test annotations here, and notably this file is currently excluded. I started by adding -> None to lots of functions, so hence my protest that we're now undoing them.
No description provided.