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

Improve error message for incompatible TypeVar value #3707

Merged
merged 3 commits into from Aug 1, 2017

Conversation

Projects
None yet
5 participants
@ilinum
Collaborator

ilinum commented Jul 12, 2017

Fixes #3341

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jul 12, 2017

Collaborator

@JukkaL what do you think of the updated error message?

Collaborator

ddfisher commented Jul 12, 2017

@JukkaL what do you think of the updated error message?

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jul 13, 2017

Collaborator

Looks reasonable! Here's another alternative that would make the error message perhaps even clearer:

Current:     Type argument 1 of "f" (TypeVar "AnyStr") cannot be "object"
Alternative: Value of type variable "AnyStr" of "f" cannot be "object"

It may also make sense to special case typing.AnyStr since it's so common. Here's an idea:

t.py:1: error: Value of type variable "AnyStr" of "f" cannot be "object"
t.py:1: note: Maybe you are mixing "str" and "bytes" values, which is not supported with "AnyStr"?
Collaborator

JukkaL commented Jul 13, 2017

Looks reasonable! Here's another alternative that would make the error message perhaps even clearer:

Current:     Type argument 1 of "f" (TypeVar "AnyStr") cannot be "object"
Alternative: Value of type variable "AnyStr" of "f" cannot be "object"

It may also make sense to special case typing.AnyStr since it's so common. Here's an idea:

t.py:1: error: Value of type variable "AnyStr" of "f" cannot be "object"
t.py:1: note: Maybe you are mixing "str" and "bytes" values, which is not supported with "AnyStr"?
@ilinum

This comment has been minimized.

Show comment
Hide comment
@ilinum

ilinum Jul 14, 2017

Collaborator

Both sounds good!

Regarding the first suggestion though, I think there might be a case where specifying type argument number is useful (when names of two type args are the same).

Collaborator

ilinum commented Jul 14, 2017

Both sounds good!

Regarding the first suggestion though, I think there might be a case where specifying type argument number is useful (when names of two type args are the same).

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 14, 2017

Collaborator

Note that there is PR #3433 that takes a more "global" approach to the AnyStr error message. @quartox are you still working on it?

Collaborator

ilevkivskyi commented Jul 14, 2017

Note that there is PR #3433 that takes a more "global" approach to the AnyStr error message. @quartox are you still working on it?

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox Jul 14, 2017

Contributor

@ilevkivskyi I will get back to it this weekend.

Contributor

quartox commented Jul 14, 2017

@ilevkivskyi I will get back to it this weekend.

@ilinum

This comment has been minimized.

Show comment
Hide comment
@ilinum

ilinum Jul 20, 2017

Collaborator

I updated the error message as per Jukka's suggestion.

However, I think the additional error message for AnyStr should be handled in #3433 since @quartox is already working on it.

Collaborator

ilinum commented Jul 20, 2017

I updated the error message as per Jukka's suggestion.

However, I think the additional error message for AnyStr should be handled in #3433 since @quartox is already working on it.

@ddfisher

LGTM!

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Aug 1, 2017

Collaborator

And agreed -- the AnyStr change can happen in #3433.

Collaborator

ddfisher commented Aug 1, 2017

And agreed -- the AnyStr change can happen in #3433.

@ddfisher ddfisher merged commit e0de8be into python:master Aug 1, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilinum ilinum deleted the ilinum:better-error-message-typevar-bound branch Aug 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment