Skip to content

Conversation

@MatthieuDartiailh
Copy link
Contributor

Closes #19551

Copy link
Member

@BvB93 BvB93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got a couple of suggestions, but things should be good to go afterwards.

@BvB93 BvB93 added 41 - Static typing 09 - Backport-Candidate PRs tagged should be backported labels Jul 23, 2021
@BvB93 BvB93 changed the title ENH: add missing dunder method to nditer type hints MAINT: add missing dunder method to nditer type hints Jul 23, 2021
@MatthieuDartiailh
Copy link
Contributor Author

I addressed all comments. Let me know if you want to see any other change.

The presence of __getattr__ is not sufficient to enable all protocols so explicitely add the dunder methods supported by nditer
@MatthieuDartiailh
Copy link
Contributor Author

I messed up when addressing the comments so I had to squash a bunch of things and forced pushed. Sorry for the mess.

Copy link
Member

@BvB93 BvB93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mattip
Copy link
Member

mattip commented Jul 25, 2021

No new tests or changes to existing tests are needed?

@MatthieuDartiailh
Copy link
Contributor Author

I can add a bunch of type checking related tests but I have no idea where they are located.

@BvB93
Copy link
Member

BvB93 commented Jul 25, 2021

See the ArrayTerator typing tests for a few examples; I'd suggest creating a new file specifically for nditer.

If you're willing to add tests, then the most import thing would be to check that the various legal statements
return the expected type (directly calling the methods is less interesting to check here). For example:

import numpy as np

nditer_obj: np.nditer

with nditer_obj as context:
    reveal_type(context)   # E: numpy.nditer

@MatthieuDartiailh
Copy link
Contributor Author

Thanks. For ``get/set/delitem`, are you fine with simply implicitly invoking the protocol but not using any reveal since everything is Any for the time being ?

@BvB93
Copy link
Member

BvB93 commented Jul 25, 2021

Thanks. For get/set/delitem, are you fine with simply implicitly invoking the protocol but not using any reveal since everything is Any for the time being ?

I'd still use a reveal_type here. The actual revealed value is less interesting here (for now),
but it does still allows for some basic verification (and checking for errors).

@MatthieuDartiailh
Copy link
Contributor Author

I pushed some first tests for nditer however I am having a hard time running tests locally. Running numpy.typing.test("slow", verbose=3), test_reveal are deselected and I could not really figure why. I would appreciate any pointer you may have.

@MatthieuDartiailh
Copy link
Contributor Author

MatthieuDartiailh commented Jul 27, 2021

Failures look unrelated to this work, I believe. Let me know if I am wrong.

@BvB93
Copy link
Member

BvB93 commented Jul 27, 2021

I pushed some first tests for nditer however I am having a hard time running tests locally. Running numpy.typing.test("slow", verbose=3), test_reveal are deselected and I could not really figure why. I would appreciate any pointer you may have.

I usually run the python ./runtests.py -t numpy/typing --mode=full command from the command line.
I think running numpy.typing.test("full") should also work.

@BvB93
Copy link
Member

BvB93 commented Jul 27, 2021

In any case, the added tests look good to me.

@mattip mattip merged commit b908da6 into numpy:main Jul 27, 2021
@mattip
Copy link
Member

mattip commented Jul 27, 2021

Thanks @MatthieuDartiailh

@MatthieuDartiailh MatthieuDartiailh deleted the nditer-typing branch July 27, 2021 12:15
@BvB93 BvB93 removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Typing of nditer: __getattr__ is not sufficient to cover __enter__, __exit___ and __iter__ dunders

3 participants