Repository navigation
Migrate remaining failure messages to mypy.messages (part1: mypy.checker) - #6118
Conversation
d4397e0 to
e659c30
Compare
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks! I like this change, have just one optional suggestion.
|
I added in some of the satellite modules for |
| if not isinstance(inferred_dunder_set_type, CallableType): | ||
| self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE, context) | ||
| self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE | ||
| .format(inferred_dunder_set_type), context) |
There was a problem hiding this comment.
The code has two variations of this error message:
"__set__ is not callable": used here"{}.__set__ is not callable": used elsewhere
I'd like to standardize on the second, but I'm not familiar enough with the code to know how to get the descriptor name here. My naive attempt above did not trigger any test errors, but it should have, so I think it's not tested. How should I proceed?
There was a problem hiding this comment.
What you are doing is not correct. I think you should use attribute_type.type.name() here, and add a test.
There was a problem hiding this comment.
This particular error is only generated in the case of an inferred __set__. I'm not sure how to trigger this. I tried a few variations along the lines of the tests in check-inference.test but all went through different code paths. Any suggestions? Is there a chance this is a dead code path?
There was a problem hiding this comment.
Yes, it looks like this is currently dead code: we only allow __set__ to be a method above, and a method (i.e. function definition) can't have a non-callable type. I think this can be replaced with an assert.
There was a problem hiding this comment.
Addressed in my latest commit
| # Too tricky, give up. | ||
| # TODO: Do something more clever here. | ||
| self.chk.fail(messages.UNSUPPORTED_ARGUMENT_2_FOR_SUPER, e) | ||
| self.chk.fail(messages.UNSUPPORTED_ARG_2_FOR_SUPER, e) |
There was a problem hiding this comment.
I renamed this constant because I wanted to standardize all of them on the shorthand "ARG" for argument, since the constants can get fairly lengthy.
| if not isinstance(inferred_dunder_set_type, CallableType): | ||
| self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE, context) | ||
| self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE | ||
| .format(inferred_dunder_set_type), context) |
There was a problem hiding this comment.
Yes, it looks like this is currently dead code: we only allow __set__ to be a method above, and a method (i.e. function definition) can't have a non-callable type. I think this can be replaced with an assert.
| T = TypeVar('T', bound=str) | ||
| class A: | ||
| def foo(self: T) -> T: # E: The erased type of self 'builtins.str' is not a supertype of its class '__main__.A' | ||
| def foo(self: T) -> T: # E: The erased type of self "builtins.str" is not a supertype of its class "__main__.A" |
There was a problem hiding this comment.
Looking at these errors, I think we should switch them to using "short form", as we do for many other errors (in a separate PR).
|
I think this should be ready to go. |
3 participants
Consolidate all mypy error messages into one place (
mypy.messages). This opens the door to some interesting features, such as adding language localization or a more systematic way of working with error messages, akin to error codes (#2417). It also helps create more consistent messages. e.g. it's now more apparent that there are a few outliers that use single quotes around type names.I'm breaking this up change up into several PRs by module, this is the first and simplest. Once all are complete, this will address #6116.
It's pretty straight-foward. The biggest challenge is coming up with good constant names. Let me know what you think.