WIP: Use assign expr for "while True: ... break" by vstinner · Pull Request #8095 · python/cpython

vstinner · 2018-07-04T22:19:40Z

Replace:

    while True:
        x = expr
        if not x:
            break
        ...

with:


    while (x := expr):
        ...

Notes:

Corner case: (x := expr) and (x != NUL), use x in the second test,
whereas x is set in the first test
asyncio/sslproto.py change avoids adding an empty chunk to appdata.
some changes remove comments
some lines are longer than 80 characters to keep everything on the
same line

Replace: while True: x = expr if not x: break ... with: while (x := expr): ... Notes: * Corner case: (x := expr) and (x != NUL), use x in the second test, whereas x is set in the first test * asyncio/sslproto.py change avoids adding an empty chunk to appdata. * some changes remove comments * some lines are longer than 80 characters to keep everything on the same line

JelleZijlstra · 2018-07-05T00:04:08Z

-            data = self.read(DEFAULT_BUFFER_SIZE)
-            if not data:
-                break
+        while (data := self.read(DEFAULT_BUFFER_SIZE)):


Why are you using parentheses here? I don't think they are syntactically necessary.

I'm too lazy to read the latest PEP, and the current implementation of the PEP is outdated, so I added parenthesis even if they are not needed.

vstinner · 2018-07-05T00:16:08Z

Delete

The point of this PR is just to open discuss on coding style: discuss when assignment expressions are appropriate or not.

The discussion: "[Python-Dev] Assignment expression and coding style: the while True case"
https://mail.python.org/pipermail/python-dev/2018-July/154323.html

"Let's say that the PEP 572 (assignment expression) is going to be approved. Let's move on and see how it can be used in the Python stdlib." :-)

I added the DO-NOT-MERGE label and "WIP" in the title. I don't want to merge this PR (at least, not as it is currently).

liiight · 2018-07-06T04:24:17Z

Thanks for this, this makes the use cases of this PEP much clearer, to me at least.

I hope the parenthesis aren't necessary though as they do feel a little unpythonic

vstinner · 2018-07-06T15:17:22Z

I hope the parenthesis aren't necessary though as they do feel a little unpythonic

Parenthesis are optional. I chose to always add them.

I don't think that we can already say if parenthesis are pythonic or not since it's a new syntax :-) We don't have enough experience with it :-)

liiight · 2018-07-06T15:35:05Z

I meant that using parenthesis in if statements is usually redundant and I'd be happy to match that behaviour with the new syntax

tim-one · 2018-07-06T16:09:23Z

The PEP explicitly rejected requiring parentheses, giving as one concrete example that they'd "look redundant" in:

# Top level in if
if match := pattern.match(line):
    return match.group(1)

Which is exactly the kind of case in which you're always adding them and are getting feedback that it just doesn't look right to experienced Python eyes. I didn't write that part or the PEP, or even participate in the discussion that resulted in it being added - I just find it to be obviously true 😉

I can't think of any case in which anyone recommends adding redundant parentheses except when clarifying evaluation order.

For example, I doubt anyone would object to the redundant parens in:

i = 1 << (2 + 1)

or

t = (1, 2)

But I can't think either of any experienced Python programmer who does champion the redundant parens in

i = (1 + 2)
i = (((1) + (2)))
return (1 + 2)
if (x or y):
while (x > y):
...

etc. The parens don't clarify anything in those - they just add counterproductive line noise. Newcomers from other languages where parens are required in such contexts often start off adding them anyway, and that's fine, but most grow out of it soon enough even if they're not slapped for it in a code review 😉

You started off by saying you wanted a discussion of coding guidelines for use of :=. Well, that's one you're getting consistent feedback on: the same as with any other bit of Python expression syntax, don't add redundant line noise unless it serves a purpose (which, for parens, is pretty much limited to forcing or clarifying evaluation order in expressions with multiple operators).

"They're optional" is no reason for adding them; "they're optional" precisely in recognition of that adding them serves no purpose in these common cases. They're just noise here.

JimJJewett

I consider these all wins, particularly where the original author had written if (whatever): break on a single line.

JimJJewett · 2018-07-06T18:16:19Z

-            j = m.end()
-            if i == j:
-                break
+        while (m := match()) and (j := m.end()) == i:


Doesn't that reverse the test on j? As in, shouldn't it be
while (m := match()) and (j := m.end()) != i:

Though I'm not sure which way this argues; the original logic was at least as hard to follow as the new code.

JimJJewett · 2018-07-06T18:21:34Z

-        while True:
-            d = ifp.read(128000)
-            if not d: break
+        while (d := ifp.read(128000)):


This is a bigger win than it looks like.

Note that the original "if not d: break" on a single line was itself arguably a style violation because the break doesn't start a line, even though it might change flow-control. That said, I would have done the same thing as the original author to keep it short; it feels like a guard instead of a real control "path".

So this is an area where vertical space matters, the "while True: " is ugly, and the current spelling is already doing slightly ugly things to avoid drawing extra emphasis ... and shortening it with the inline assignment improves on all three.

JimJJewett · 2018-07-06T18:31:09Z

-                    if this is None or this == "\n":
-                        break
+                while (this := sourceget()) is not None and (this != "\n"):
+                    pass


In this case, you don't even need "this" again, because the next instruction continues the enclosing loop, which resets this. (Essentially, "this" was a dummy name, getting re-used.)

while sourceget() not in (None, "\n"): pass

JimJJewett · 2018-07-06T18:42:04Z

-                s = self.__read(1)
-                if not s or s == NUL:
-                    break
+            while (s := self.__read(1)) and s != NUL:


I know the second part (s != NUL) is controversial -- but consider this one vote saying "yes, this was still a good replacement". The meaning behind the line is "throw away characters until you get to the end of the string", and splitting the test into two parts is just an distraction (needed because of how the end of string happens to be defined for this function).

The existing 4 lines make it seem bigger than it is, but so would the three-line alternative suggested by Tim. Frankly, even the 2nd line for "pass" seems slightly misleading, but I don't see a further improvement.

Also note that s is really a dummy variable, only needed to allow comparisons against a 2nd type of string-end. Same comments for flag&16 -- if there is a difference in the cases, the length caused me to miss it.

JimJJewett · 2018-07-06T19:01:47Z

-                buf = self._read(self.bufsize)
-                if not buf:
-                    break
+            while (buf := self._read(self.bufsize)):


8d13091 has eliminated this code (and just asserted that users will always call with a non-None size.)

JimJJewett · 2018-07-06T19:13:01Z

-            if tarinfo is None:
-                break
+        while (tarinfo := self.next()) is not None:
+            pass


This is another controversial change that I like. The first one looks like it is doing some sort of work to select or filter; the 2nd makes it more clear that it is just skipping through (but touching and therefore loading?) all the self.next() chunks.

That said, the tarfile temp name is not used again. It might be even more clear as

while self.next() is not None: pass

vstinner · 2018-07-12T07:53:54Z

The PEP 572 has been accepted. I only wrote these WIP PEPs to help me to discuss the PEP. I never intended to propose these changes to be merged. So I close this PR.

mgedmin · 2018-07-18T13:00:47Z

-            if not line:
-                break
+        while (line := sys.stdin.readline()):
            self.write(line.encode('ascii'))


TBH for line in sys.stdin: seems better to me here.

vstinner added the DO-NOT-MERGE label Jul 4, 2018

vstinner requested review from a team, 1st1, asvetlov, rhettinger and skrah as code owners July 4, 2018 22:19

the-knights-who-say-ni added the CLA signed label Jul 4, 2018

bedevere-bot added the awaiting merge label Jul 4, 2018

JelleZijlstra reviewed Jul 5, 2018

View reviewed changes

This was referenced Jul 5, 2018

WIP: Use assign expr for match/group #8097

Closed

WIP: Use assign expr in list comprehension #8098

Closed

WIP: Use assign expr with "elif" #8115

Closed

WIP: use assignment expression in stdlib (combined PR) #8122

Closed

JimJJewett reviewed Jul 6, 2018

View reviewed changes

vstinner closed this Jul 12, 2018

vstinner deleted the wip_while_true_assign_expr branch July 12, 2018 07:54

mgedmin reviewed Jul 18, 2018

View reviewed changes

Uh oh!

Conversation

vstinner commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jul 5, 2018

Uh oh!

liiight commented Jul 6, 2018

Uh oh!

vstinner commented Jul 6, 2018

Uh oh!

liiight commented Jul 6, 2018

Uh oh!

tim-one commented Jul 6, 2018

Uh oh!

JimJJewett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jul 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vstinner commented Jul 4, 2018 •

edited

Loading