Fix #2070 - bytes formatting incorrect in python 3 by buckbaskin · Pull Request #2168 · python/mypy

buckbaskin · 2016-09-22T05:25:30Z

This pull request aims to fix the type checking of bytes strings. The code previously used a single path for both bytes and strings, which broke the example given in the bug (#2070) that expected a bytes output to a function for example. This pull request creates a separate method for handling bytes in the same pattern as strings, but with the correct type output.
At the time the pull request is created, there may be some errors in the types of inputs into the bytes % interpolation because it is using the type checking logic for string interpolation.

gvanrossum · 2016-09-22T18:47:56Z

Thanks! This looks fine, but I have a few requests:

Can you refactor check_str_interpolation() and check_bytes_interpolation() to reduce the code duplication? Maybe there should be just one function that doesn't return anything, and instead the return type should just be picked by the caller (that seems easy enough).
I don't like the cast. It would be better to pass in the string value and node context separately.
It seems the code here never supported UnicodeExpr, but in Python 2, expressions like u'I have %d shrubberies' % 12 should also be checked. Should be easy enough.

- check_str_interpolation accepts StrExpr, BytesExpr, UnicodeExpr - remove a cast to a StrExpr - Add "support" for UnicodeExpr * Note that this does not implement special checks for differences between valid StrExpr and BytesExpr

gvanrossum

This would explain your test failure.

gvanrossum · 2016-09-26T03:52:19Z

mypy/checkexpr.py

-                return self.strfrm_checker.check_byte_interpolation(e.left, e.right)
-            elif isinstance(e.left, StrExpr):
-                return self.strfrm_checker.check_str_interpolation(cast(StrExpr, e.left), e.right)
+            return self.strfrm_checker.check_str_interpolation(e.left, e.right)


That can't be right? What if e.left is not a string at all?

I added the isinstance check back. It was my oversite that non StrExpr-like objects would get through to this point.

gvanrossum · 2016-09-26T04:11:21Z

mypy/checkstrformat.py

+    def check_str_interpolation(self, str: Union[StrExpr, BytesExpr, UnicodeExpr], replacements: Node) -> Type:
        """Check the types of the 'replacements' in a string interpolation
        expression: str % replacements
        """


Can you add a TODO around here that in Python 3 the bytes format characters are actually more restricted than str/unicode format characters? (a) There are fewer supported characters; (b) %s only supports bytes. (In Python 2 these limitations do not apply.)

Also bytes formatting in Python 3 is only supported in 3.5 and up.

Is the bytes formatting in 3.5 a TODO? Or is it something that I'll need to check for in code?

gvanrossum · 2016-09-26T04:20:12Z

Up to you. If you have time to learn how to do it it would be great to add
this check (there's a pyversion variable used throughout the checker).

On Sun, Sep 25, 2016 at 9:16 PM, Buck Baskin notifications@github.com
wrote:

@buckbaskin commented on this pull request.

In mypy/checkstrformat.py #2168:

@@ -55,7 +55,7 @@ def init(self,
self.exprchk = exprchk
self.msg = msg

def check_str_interpolation(self, str: StrExpr, replacements: Node) -> Type:

def check_str_interpolation(self, str: Union[StrExpr, BytesExpr, UnicodeExpr], replacements: Node) -> Type:
"""Check the types of the 'replacements' in a string interpolation
expression: str % replacements
"""

Is the bytes formatting in 3.5 a TODO? Or is it something that I'll need
to check for in code?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2168, or mute the thread
https://github.com/notifications/unsubscribe-auth/ACwrMlh-1BQHpJUtTe_S8SGDcrERQPZcks5qt0c4gaJpZM4KDicI
.

--Guido van Rossum (python.org/~guido)

gvanrossum · 2016-09-26T15:49:23Z

Thank you for the patch! Are you interested in doing any of the follow-up TODOs?

buckbaskin · 2016-09-26T17:28:27Z

I was planning on looking into the pyversion check next. What is the preferred process for completing a TODO? Should I file a bug and then make a pull request to fix it?

gvanrossum · 2016-09-26T17:31:58Z

You can just send the PR, no issue required.

--Guido (mobile)

On Sep 26, 2016 10:29 AM, "Buck Baskin" notifications@github.com wrote:

I was planning on looking into the pyversion check next. What is the
preferred process for completing a TODO? Should I file a bug and then make
a pull request to fix it?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#2168 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACwrMrd0lTGvx44n0WhhQQqxF2vOO8Knks5quAC9gaJpZM4KDicI
.

Buck Baskin added 5 commits September 14, 2016 12:09

Add a code example for the typing.IO module

34ffd12

Merge branch 'master' of https://github.com/python/mypy

190a120

Merge branch 'master' of https://github.com/python/mypy

ca645f2

Add a parallel path for bytes and str for checking % interpolation

818adf7

Update the expected type of first arg to check_byte_interpolation

76d6d71

buckbaskin closed this Sep 22, 2016

buckbaskin reopened this Sep 22, 2016

Refactor code to reduce duplication in changes

9920e9b

- check_str_interpolation accepts StrExpr, BytesExpr, UnicodeExpr - remove a cast to a StrExpr - Add "support" for UnicodeExpr * Note that this does not implement special checks for differences between valid StrExpr and BytesExpr

gvanrossum requested changes Sep 26, 2016

View reviewed changes

Buck Baskin added 2 commits September 26, 2016 00:05

Reinstate type check for StrExpr, etc

af5dc00

Add byte interpolation unit test cases

3ce554c

gvanrossum reviewed Sep 26, 2016

View reviewed changes

Add TODO comments re bytes formatting functionality

b769a8f

Buck Baskin and others added 5 commits September 26, 2016 00:24

Fix two lint errors in checkstrformat

1c1d18b

Add a single Unicode python2 test

31c7412

Clean up trailing comma.

9a9b4ae

Add periods to sentences.

41bce18

Don't assume.

592e456

gvanrossum approved these changes Sep 26, 2016

View reviewed changes

gvanrossum merged commit 2fbf387 into python:master Sep 26, 2016

buckbaskin mentioned this pull request Sep 28, 2016

Use pyversion variable to change bytes checking for python 3 less than 3.5 #2191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #2070 - bytes formatting incorrect in python 3#2168

Fix #2070 - bytes formatting incorrect in python 3#2168
gvanrossum merged 14 commits intopython:masterfrom
buckbaskin:fixBytes

buckbaskin commented Sep 22, 2016

Uh oh!

gvanrossum commented Sep 22, 2016

Uh oh!

gvanrossum left a comment

Uh oh!

gvanrossum Sep 26, 2016

Uh oh!

buckbaskin Sep 26, 2016

Uh oh!

gvanrossum Sep 26, 2016

Uh oh!

gvanrossum Sep 26, 2016

Uh oh!

buckbaskin Sep 26, 2016

Uh oh!

gvanrossum commented Sep 26, 2016

@buckbaskin commented on this pull request.

Uh oh!

gvanrossum commented Sep 26, 2016

Uh oh!

buckbaskin commented Sep 26, 2016

Uh oh!

gvanrossum commented Sep 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Uh oh!

Conversation

buckbaskin commented Sep 22, 2016

Uh oh!

gvanrossum commented Sep 22, 2016

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum Sep 26, 2016

Choose a reason for hiding this comment

Uh oh!

buckbaskin Sep 26, 2016

Choose a reason for hiding this comment

Uh oh!

gvanrossum Sep 26, 2016

Choose a reason for hiding this comment

Uh oh!

gvanrossum Sep 26, 2016

Choose a reason for hiding this comment

Uh oh!

buckbaskin Sep 26, 2016

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Sep 26, 2016

@buckbaskin commented on this pull request.

Uh oh!

gvanrossum commented Sep 26, 2016

Uh oh!

buckbaskin commented Sep 26, 2016

Uh oh!

gvanrossum commented Sep 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments