WIP: use assignment expression in stdlib (combined PR) by vstinner · Pull Request #8122 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation29 Commits6 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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
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).
| line = input.readline() |
|---|
| if not line: |
| break |
| while (line := input.readline()): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"for line in input:" and "while (line := readline()):" are not strictly the same: input iterator can use something different than readline...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have assumed it was catering to old file-like objects that didn't support iteration at all.
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.
Thanks a lot for doing this. I think it'd be more fair to remove parenthesis though, at least for if expressions.
I chose to add parenthesis even when they are optional. So please try to ignore parenthesis :-)
| break |
|---|
| else: |
| n = n + q >> 1 |
| while (q := c//n) < n: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where you went too far :)
** flies away **
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What? I think this is the change with the most win in the PR!
kissgyorgy, shangxiao, motleytech, petergaultney, amcgregor, bb010g, nihn, alexvicegrab, and cra reacted with thumbs up emoji DylanDmitri, lithp, satejsoman, and cra reacted with hooray emoji cra reacted with heart emoji cra reacted with rocket emoji
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| ans = self._check_nans(other, context=context) |
|---|
| if ans: |
| return ans |
| if (self._is_special or other._is_special |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs parens to isolate the "or" part.
if (self._is_special or other._is_special)
and (ans := self._check_nans(other, context=context)):
| @@ -196,11 +196,8 @@ def feed_ssldata(self, data, only_handshake=False): |
|---|
| if self._state == _WRAPPED: |
| # 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)): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was previously noted in #8095
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last (missing) appdata.append(chunk) can go into the else: clause of the while.
| @@ -1099,8 +1093,7 @@ def make_encoding_map(decoding_map): |
|---|
| # Tell modulefinder that using codecs probably needs the encodings |
| # package |
| _false = 0 |
| if _false: |
| if (_false := 0): |
| import encodings |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks less intuitive to me; not that the old code was intuitive, though.
| @@ -594,9 +593,8 @@ def __repr__(self): |
|---|
| info.append(f'fd={self._fileno}') |
| selector = getattr(self._loop, '_selector', None) |
| 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: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
| status = _overlapped.GetQueuedCompletionStatus(self._iocp, ms) |
|---|
| if status is None: |
| break |
| while (status := _overlapped.GetQueuedCompletionStatus(self._iocp, ms)) is not None: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| line = input.readline() |
|---|
| if not line: |
| break |
| while (line := input.readline()): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have assumed it was catering to old file-like objects that didn't support iteration at all.
| @@ -818,8 +817,7 @@ def effective(file, line, frame): |
|---|
| # Ignore count applies only to those bpt hits where the |
| # condition evaluates to true. |
| try: |
| val = eval(b.cond, frame.f_globals, frame.f_locals) |
| if val: |
| if eval(b.cond, frame.f_globals, frame.f_locals): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -1099,8 +1093,7 @@ def make_encoding_map(decoding_map): |
|---|
| # Tell modulefinder that using codecs probably needs the encodings |
| # package |
| _false = 0 |
| if _false: |
| if (_false := 0): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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__.
| rv = reductor() |
|---|
| else: |
| raise Error("un(shallow)copyable object of type %s" % cls) |
| raise Error("un(shallow)copyable object of type %s" % cls) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
| @@ -886,35 +886,31 @@ def __lt__(self, other, context=None): |
|---|
| self, other = _convert_for_comparison(self, other) |
| if other is NotImplemented: |
| return other |
| ans = self._compare_check_nans(other, context) |
| if ans: |
| if self._compare_check_nans(other, context): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't even use the assignment expression
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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).
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.
| if (mo := re.match(r'.*"([^"]+)"$', ml)): |
|---|
| path = mo.group(1) |
| else: |
| path = ml.split()[-1] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -638,8 +637,7 @@ def close(self): |
|---|
| """ |
| self.acquire() |
| try: |
| sock = self.sock |
| if sock: |
| if (sock := self.sock): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| 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()))] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet refactor!
| j = m.end() |
|---|
| if i == j: |
| break |
| while (m := match()) and (j := m.end()) == i: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
== should be !=
@vstinner
I was a little late to notice that this is already closed. Kindly ignore my comments on the code.