Skip to content
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 bytes formatting error #14959

Merged
merged 3 commits into from
Mar 29, 2023
Merged

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Mar 26, 2023

Fixes #11806

This error message was last changed in #11139 to make it more applicable to f-strings. Since users are still getting confused, it makes sense to be more explicit about what the recommended fix is. There was also an extra set of quotes and a mention of Python 3 that didn't help.

I think this is still very much a useful error, it caught a bug I had recently.

Fixes python#11806

This error message was last changed in python#11139 to make it more applicable
to f-strings. Since users are still getting confused, it makes sense to
be more explicit about what the recommended fix is. There was also an
extra set of quotes and a mention of Python 3 that didn't help.
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

This looks like a big improvement in my opinion!

Should we maybe also improve some of the docs around this error code, like Jukka suggested in #11806 (comment), before closing the issue, though?

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

steam.py (https://github.com/Gobot1234/steam.py)
- steam/client.py:814: error: On Python 3 formatting "b'abc'" with "{}" produces "b'abc'", not "abc"; use "{!r}" if this is desired behavior  [str-bytes-safe]
+ steam/client.py:814: error: If x = b'abc' then f"{x}" or "{}".format(x) produces "b'abc'", not "abc". If this is desired behavior, use f"{x!r}" or "{!r}".format(x). Otherwise, decode the bytes  [str-bytes-safe]

@AlexWaygood
Copy link
Member

error: If x = b'abc' then f"{x}" or "{}".format(x) produces "b'abc'", not "abc".

I know it's not the usual mypy style for error messages, but I do think this would be much clearer here if backticks were used to indicate the code snippets (the fact that or is a Python keyword as well as a word in the English language isn't very helpful for this error message).

I.e. I'd find this even clearer:

error: If `x = b'abc'` then `f"{x}"` or `"{}".format(x)` produces `"b'abc'"`, not `"abc"`.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

I agree with Alex, we also need to document it better.

Thanks for taking this over!

@@ -638,8 +638,8 @@ def g() -> int:
'%d' % 'no' # E: Incompatible types in string interpolation (expression has type "str", placeholder has type "Union[int, float, SupportsInt]") [str-format]
'%d + %d' % (1, 2, 3) # E: Not all arguments converted during string formatting [str-format]

'{}'.format(b'abc') # E: On Python 3 formatting "b'abc'" with "{}" produces "b'abc'", not "abc"; use "{!r}" if this is desired behavior [str-bytes-safe]
'%s' % b'abc' # E: On Python 3 formatting "b'abc'" with "%s" produces "b'abc'", not "abc"; use "%r" if this is desired behavior [str-bytes-safe]
'{}'.format(b'abc') # E: If x = b'abc' then f"{x}" or "{}".format(x) produces "b'abc'", not "abc". If this is desired behavior, use f"{x!r}" or "{!r}".format(x). Otherwise, decode the bytes [str-bytes-safe]
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about injecting real user's values into the error message?

'On Python 3 formatting "b\'abc\'" with "{}" '
'produces "b\'abc\'", not "abc"; '
'use "{!r}" if this is desired behavior',
'If x = b\'abc\' then f"{x}" or "{}".format(x) produces "b\'abc\'", '
Copy link
Member

Choose a reason for hiding this comment

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

Sadly, we have no way of telling whether it is a .format call or fstring. It would really help with the error message.

Copy link
Member

Choose a reason for hiding this comment

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

Is that because we desugar f-strings into a CallExpr that looks just like a str.format call? Maybe we can stash some data in the CallExpr to indicate that it derived from an f-string.

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.

FWIW I like the new error message.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Agree this is nicer, would be even better if we can have separate f-string error messages.

hauntsaninja added a commit to hauntsaninja/mypy that referenced this pull request Mar 29, 2023
@hauntsaninja hauntsaninja merged commit ddf0c05 into python:master Mar 29, 2023
@hauntsaninja hauntsaninja deleted the bytes-err branch March 29, 2023 03:51
@hauntsaninja
Copy link
Collaborator Author

Thanks all!

Should we maybe also improve some of the docs around this error code

Opened #14959 for this

I know it's not the usual mypy style for error messages

I think probably okay as is, backticks aren't used anywhere else in mypy and this is an error message where additional visual noise seems harmful. Not sure how familiar users are with markdown either...

injecting real user's values into the error message

We won't always have the bytes value at hand. I think the example "abc" makes it clearer that the problem isn't with the specific value, just the act of formatting

telling whether it is a .format call or fstring

I agree that this would be nice

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

Successfully merging this pull request may close these issues.

str-bytes-safe with new Formatted string literals
5 participants