Fix type annotations for get_axis_num (GH 9822)

[bmerry]

Explicitly annotate that a single str argument leads to an int return, overriding the match against Iterable.

• Closes DataArray.get_axis_num is incorrectly typed #9822
• User visible changes (including notable bug fixes) are documented in whats-new.rst

I haven't added tests since I didn't spot any existing tests of type annotations, but let me know if I missed them.

[welcome]

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

[bmerry]

Hmm, seems mypy isn't liking that in CI, although it didn't complain when I used it to verify my MCVE. I'll investigate and re-open when I've figured out why.

[bmerry]

Ok, the mypy error is explained in more detail here. The problem is that when passing a variable that's statically known to be Iterable[str], you get back either str or tuple[str, ...] depending on the run-time type. Thus, to be technically correct, one should annotate the return type in this case to be str | tuple[str, ...]. However, that's not very pragmatic, because for most practical uses, the argument type will be known more specifically (e.g. list[str] or tuple[str]), and so you know you're getting back a tuple.

So I've done the pragmatic thing and just suppressed the mypy error. The documentation linked above says

Note that in cases where you ignore the overlapping overload error, mypy will usually still infer the types you expect at callsites.

[headtr1ck]

Would it not be enough to reverse the order of the existing overloads?

Or will that mess up tuples (which are hashable)?

[bmerry]

Would it not be enough to reverse the order of the existing overloads?

Or will that mess up tuples (which are hashable)?

I haven't tested it, but I assume you'd run into that problem.

[max-sixty]

Thanks a lot for the contribution @bmerry !

Would it be possible to add a test function which uses these? If the test function is annotated with def test_foo() -> None, then mypy will check it. That way we can test that this fixes the issue, and other can try improving it (like the swapping overload order above) and we can be confident it won't regress.

[bmerry]

Thanks a lot for the contribution @bmerry !

Would it be possible to add a test function which uses these? If the test function is annotated with def test_foo() -> None, then mypy will check it. That way we can test that this fixes the issue, and other can try improving it (like the swapping overload order above) and we can be confident it won't regress.

If I have time I'll give it a go, but to be honest I'm not sure I will. The test itself should be fairly easy to write, but setting up a test environment will have a learning curve since I've not used conda before and I don't want to risk it getting into a fight with pyenv over my PYTHON_PATH.

[max-sixty]

Yeah that's fair. Being able to make small contributions is important, and I think this is a good enough win that it's worth merging.

If you manage to set up an environment feel free to add a test to immortalize your contribution! (and I use pip to avoid the overhead, you might find it easier...)

[welcome]

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again!

[bmerry]

and I use pip to avoid the overhead, you might find it easier...

That sounds good. What arguments are you passing to pip to install all the right dependencies?

[max-sixty]

That sounds good. What arguments are you passing to pip to install all the right dependencies?

pip install .[dev] works!

[headtr1ck]

pip install .[dev] works!

Try pip install -e .[dev] if you intend to develop xarray and not just use it.