-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fixed NotImplementedType regression #14966
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
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: static-frame (https://github.com/static-frame/static-frame)
+ static_frame/core/node_values.py:98: error: Unused "type: ignore" comment [unused-ignore]
+ static_frame/core/node_values.py:272: error: Unused "type: ignore" comment [unused-ignore]
|
|
@srittau This should do the trick. |
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.
Does this intentionally drop the __call__: None?
|
@hauntsaninja I believe this is/was just some relic for annotating that this class couldn't be instantiated before |
|
iirc it was a trick that gave better error messages when users did raise notimplemented() instead of raise notimplementederror() maybe worth re-running pyright and mypy to see what the errors are like these days |
|
Oh, is that a thing? I've never even considered raising >>> raise NotImplemented()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'NotImplementedType' object is not callable
>>> raise NotImplemented
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: exceptions must derive from BaseException |
|
Ok, so I just tested the following: def testA() -> None:
raise NotImplemented()
def testB() -> None:
raise NotImplementedwith results (this PR)
vs commit 8aaa86f (before this PR)
|
|
When re-adding this, we should leave a comment explaining why it's there. |
|
Okay, looks like mypy in the intervening years added some special casing here. This explains your last bullet point; the special casing was added assuming a fully resolved name of builtins._NotImplementedType. Given mypy has some special casing now and pyright's behaviour is unaffected, let's just do the simpler thing and remove this in both places. I opened #14971 to make the definitions consistent in typeshed. Also opening a PR to mypy to fix up the special casing |
|
mypy PR here python/mypy#20168 |
See also discussion on python/typeshed#14966
Fixes #14965