WIP: use assignment expression in stdlib (combined PR) by vstinner · Pull Request #8122 · python/cpython

vstinner · 2018-07-05T21:30:42Z

Combine all my PR which attempt to modify the stdlib to see where using assignment expressions (PEP 572) would be appropriate:

Modify for loops and list comprehensions to use assignment expressions.

Replace: else: var = expr if var: ... with: elif (var := expr): ...

Replace: var = expr if var: ... with: if (var := expr): ... Don't replace when: * var is used after the if * Code like "var = regex.match(); if var: return var.group(1)" is left unchanged since it's covered by a dedicated PR. Sometimes, var is only used in the condition and so has been removed in this change. Maybe such kind of pattern should be addressed in a different pull request. This patch is restricted to the simplest test "if var:", other conditions like "if var > 0:" are left unchaned to keep this change reviewable (short enough).

Replace: m = regex.match(text) if m: return m.group(1) with: if (m := regex.match(text)): return m.group(1) Notes: * Similar code using "if not m: return" is unchanged * When the condition was more complex than "if m", the code is left unchanged

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

vstinner · 2018-07-05T21:31:20Z

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).

RonnyPfannschmidt · 2018-07-06T08:54:46Z

-        line = input.readline()
-        if not line:
-            break
+    while (line := input.readline()):


i would like to make a point of just using an actual for loop for those case simply not to litter with missuses of the feature where applicable

"for line in input:" and "while (line := readline()):" are not strictly the same: input iterator can use something different than readline...

I would have assumed it was catering to old file-like objects that didn't support iteration at all.

ndevenish · 2018-07-06T10:50:19Z

I feel like I'm being picky, but since this is about style recommendations... in e.g.

if (nlines := rawdata.count("\n", i, j)):

My understanding was that the outer brackets were only necessary/desirable when using as a compound expression e.g. amending this example to the contrived

if (nlines := rawdata.count("\n", i, j)) == 10:

Have I just misread the PEP or is the intended style for these to always parenthesis-wrap them, even when unnecessary? (Or, am I misinterpreting the intention of this pull...)

Edit: Another comment referred me to Tim Peter's mailing list comment and the section in the PEP about parentheses, so I guess this is just some artifact of whatever transform was applied to the code.

giampaolo · 2018-07-06T14:59:25Z

Thanks a lot for doing this. I think it'd be more fair to remove parenthesis though, at least for if expressions.

vstinner · 2018-07-06T15:14:56Z

I chose to add parenthesis even when they are optional. So please try to ignore parenthesis :-)

alesdakshanin · 2018-07-06T16:51:36Z

-                break
-            else:
-                n = n + q >> 1
+        while (q := c//n) < n:


This is where you went too far :)
** flies away **

What? I think this is the change with the most win in the PR!

I agree with @phord on this one.

The places replacing the whole while: True followed by an assignment and a conditional break looks much cleaner to me now.

I was slightly sceptic at first but I’m warming up to this proposal now.

phord · 2018-07-06T20:56:13Z

-            ans = self._check_nans(other, context=context)
-            if ans:
-                return ans
+        if (self._is_special or other._is_special


Needs parens to isolate the "or" part.

if (self._is_special or other._is_special) and (ans := self._check_nans(other, context=context)):

phord · 2018-07-06T21:08:10Z

                # Main state: read data from SSL until close_notify
-                while True:
-                    chunk = self._sslobj.read(self.max_size)
+                while (chunk := self._sslobj.read(self.max_size)):


The new code calls appdata.append(chunk) one fewer time than the old code did. It used to send the final "not chunk", but now it doesn't.

Yeah this was previously noted in #8095

The last (missing) appdata.append(chunk) can go into the else: clause of the while.

phord · 2018-07-06T21:14:08Z

-_false = 0
-if _false:
+if (_false := 0):
    import encodings


This looks less intuitive to me; not that the old code was intuitive, though.

JimJJewett · 2018-07-06T20:54:12Z

        if self._pipe is not None and selector is not None:
-            polling = selector_events._test_selector_event(
-                selector, self._fileno, selectors.EVENT_WRITE)
-            if polling:


In this case, I'm not sure it is an improvement. The function call is the main point, and the if-test and really just to change how status is reported. So moving the important call inside the "if" is a net negative. (The fact that it is shorter means I might do it anyhow, so that may be a slight argument against the proposal, instead of just this particular change.)

JimJJewett · 2018-07-06T21:06:03Z

-            status = _overlapped.GetQueuedCompletionStatus(self._iocp, ms)
-            if status is None:
-                break
+        while (status := _overlapped.GetQueuedCompletionStatus(self._iocp, ms)) is not None:


This is another case where None is the only falsey value, and it might be simpler to shorten things by assuming that.

while status := _overlapped.GetQueuedCompletionStatus(self._iocp, ms):

Replacing "if status is None" with "if not status" doesn't lead to the same mental simplification. So for those who like comparing directly against None as a future-proofing device, this might be an argument against the new syntax.

JimJJewett · 2018-07-06T21:09:27Z

-        line = input.readline()
-        if not line:
-            break
+    while (line := input.readline()):


I would have assumed it was catering to old file-like objects that didn't support iteration at all.

JimJJewett · 2018-07-06T21:13:05Z

            try:
-                val = eval(b.cond, frame.f_globals, frame.f_locals)
-                if val:
+                if eval(b.cond, frame.f_globals, frame.f_locals):


The original code is the sort of thing I end up with when I'm intentionally breaking lines up for debugging. The possibility of inserting "val := " instead of a whole new line means I wouldn't ever have split it out to two lines. Whether that is a good thing or not may depend on taste.

JimJJewett · 2018-07-06T21:48:55Z

 # package
-_false = 0
-if _false:
+if (_false := 0):


This change is bad. I think the purpose if the "if _false" was to prevent the code from running, while still making the name available for static analysis, such as during compilation. This change just makes it more likely to get stripped out by an optimizer. And of course, there is always a chance that someone really is toggling the module-level variable _false from outside.

As a quick note as I browse this PR way, way after the fact:

there is always a chance that someone really is toggling the module-level variable _false from outside

Not in this scenario. The developer importing this module would need to pause execution between the assignment and conditional to modify the value successfully. Some form of pre-assignment would just get overridden with zero, and re-assignment later would not re-evaluate the conditional on subsequent imports. I'd hope and expect static optimizers to eliminate the conditional body in a similar vein to if __debug__.

JimJJewett · 2018-07-06T21:55:43Z

-                rv = reductor()
-            else:
-                raise Error("un(shallow)copyable object of type %s" % cls)
+        raise Error("un(shallow)copyable object of type %s" % cls)


ah... this might be the one I was looking for when I reviewed the "if" pull request. The change is a clear improvement, even if a less mechanical rewrite would be better still. (And I admit that "clear improvement" probably isn't enough to make a change to existing code at this point.)

graingert · 2018-07-06T22:37:28Z

            return other
-        ans = self._compare_check_nans(other, context)
-        if ans:
+        if self._compare_check_nans(other, context):


This doesn't even use the assignment expression

@graingert
Correct.

It seems to have been done to keep this code similar to the other places below where the assignment expression is used in very similar circumstances (and its also a minor cleanup as the ans is not really required here).

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

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.

motleytech · 2018-08-11T07:02:29Z

+            if (mo := re.match(r'.*"([^"]+)"$', ml)):
+                path = mo.group(1)
+            else:
+                path = ml.split()[-1]


The old code could have used the ternary

path = mo.group(1) if mo else ml.split()[-1]

However, the replacement for that would now look like

path = mo.group(1) if (mo := re.match(r'.*"([^"]+)"$', ml)) else ml.split()[-1]

I think that loses some readability because of the complicated if condition. Using the if else statement seems better here.

motleytech · 2018-08-11T07:13:28Z

        try:
-            sock = self.sock
-            if sock:
+            if (sock := self.sock):


I wonder if this code can be written as

if self.sock: self.sock.close() self.sock = None

or is there some importance to the particular order to the setting to None and closing the socket? I was thinking that any race condition should be avoided due to the self.acquire

motleytech · 2018-08-11T07:19:36Z

-                lines.append(match.group(1, 2))
+        lines = [match.group(1, 2)
+                 for raw_line in raw_lines
+                 if (match := line_pat.search(raw_line.strip()))]


Sweet refactor!

motleytech · 2018-08-11T07:27:18Z

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


== should be !=

motleytech · 2018-08-11T07:47:30Z

@vstinner
I was a little late to notice that this is already closed. Kindly ignore my comments on the code.

vstinner added 6 commits July 5, 2018 23:24

WIP: Use assign expr in list comprehension

d847324

Modify for loops and list comprehensions to use assignment expressions.

WIP: Use assign expr with "elif"

8e01564

Replace: else: var = expr if var: ... with: elif (var := expr): ...

Fix zipfile

057640a

WIP: Use assign expr for match/group

d941686

Replace: m = regex.match(text) if m: return m.group(1) with: if (m := regex.match(text)): return m.group(1) Notes: * Similar code using "if not m: return" is unchanged * When the condition was more complex than "if m", the code is left unchanged

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

vstinner requested review from a team, 1st1, asvetlov, ericvsmith, rhettinger and skrah as code owners July 5, 2018 21:30

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

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

RonnyPfannschmidt reviewed Jul 6, 2018

View reviewed changes

alesdakshanin reviewed Jul 6, 2018

View reviewed changes

phord reviewed Jul 6, 2018

View reviewed changes

JimJJewett reviewed Jul 6, 2018

View reviewed changes

graingert reviewed Jul 6, 2018

View reviewed changes

vstinner closed this Jul 12, 2018

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

motleytech reviewed Aug 11, 2018

View reviewed changes

Comment thread Lib/re.py

j = m.end()

if i == j:

break

while (m := match()) and (j := m.end()) == i:

motleytech Aug 11, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

== should be !=

Uh oh!

Conversation

vstinner commented Jul 5, 2018

Uh oh!

vstinner commented Jul 5, 2018

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!

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

Uh oh!

Uh oh!

giampaolo commented Jul 6, 2018

Uh oh!

vstinner commented Jul 6, 2018

Uh oh!

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

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!

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!

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!

motleytech Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

motleytech commented Aug 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

ndevenish commented Jul 6, 2018 •

edited

Loading

alesdakshanin Jul 6, 2018 •

edited

Loading

motleytech Aug 11, 2018 •

edited

Loading