-
-
Notifications
You must be signed in to change notification settings - Fork 12k
MAINT: add missing dunder method to nditer type hints #19554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
BvB93
left a comment
There was a problem hiding this 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.
|
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
6b1a3f8 to
6ffa543
Compare
|
I messed up when addressing the comments so I had to squash a bunch of things and forced pushed. Sorry for the mess. |
BvB93
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
No new tests or changes to existing tests are needed? |
|
I can add a bunch of type checking related tests but I have no idea where they are located. |
|
See the If you're willing to add tests, then the most import thing would be to check that the various legal statements import numpy as np
nditer_obj: np.nditer
with nditer_obj as context:
reveal_type(context) # E: numpy.nditer |
|
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 |
|
I pushed some first tests for nditer however I am having a hard time running tests locally. Running |
|
Failures look unrelated to this work, I believe. Let me know if I am wrong. |
I usually run the |
|
In any case, the added tests look good to me. |
|
Thanks @MatthieuDartiailh |
Closes #19551