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

Remove "builtins." from output for overloads and builtins.None #5073

Merged
merged 5 commits into from May 18, 2018

Conversation

Projects
None yet
2 participants
@JelleZijlstra
Collaborator

JelleZijlstra commented May 17, 2018

Part of #5072.

Overloads are probably the most common user-facing feature that still outputs "builtins" in error messages; this diff fixes them to use standard type formatting.

I also change builtins.None to None, since builtins.None is a syntax error. A comment claimed that we use builtins.None to distinguish the type from the value, but I don't think there are a lot of contexts where that confusion is likely.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented May 17, 2018

Thanks for cleaning this up! I would propose to change the error format to something like this:

  • No overload variant of "f" of "A" matches argument type "B" if there is a single argument
  • No overload variant of "f" of "A" matches argument types "A", "B", "C" otherwise.

(Note that quotes are used by some editor plugins to better highlight types/methods in error messages.)

@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented May 17, 2018

Yes, that would be better. What do you suggest for the case where the list is empty? (All overloads take at least one argument, but the user called f().) I suggest All overloads of "f" require at least one argument.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented May 17, 2018

Yes this makes sense.

@ilevkivskyi

LGTM! Unless there are some objections, I will merge this soon.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented May 17, 2018

(also error message in tests needs to be updated)

@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented May 17, 2018

Oops, just pushed a commit fixing the two tests I messed up.

@ilevkivskyi ilevkivskyi merged commit d6566be into python:master May 18, 2018

2 checks passed

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

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:nobuiltins branch May 18, 2018

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