Skip to content

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

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

Merged
merged 5 commits into from
May 18, 2018

Conversation

JelleZijlstra
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

Yes this makes sense.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ilevkivskyi
Copy link
Member

(also error message in tests needs to be updated)

@JelleZijlstra
Copy link
Member Author

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

@ilevkivskyi ilevkivskyi merged commit d6566be into python:master May 18, 2018
@JelleZijlstra JelleZijlstra deleted the nobuiltins branch May 18, 2018 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants