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

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

Closed
wants to merge 6 commits into from

Conversation

@vstinner
Copy link
Member

commented Jul 5, 2018

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

vstinner added some commits Jul 5, 2018

WIP: Use assign expr in list comprehension
Modify for loops and list comprehensions to use assignment
expressions.
WIP: Use assign expr with "elif"
Replace:

    else:
        var = expr
        if var:
            ...

with:

    elif (var := expr):
        ...
WIP: use assign expr on if
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).
WIP: Use assign expr for match/group
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
WIP: Use assign expr for "while True: ... break"
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

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

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()):

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Jul 6, 2018

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

This comment has been minimized.

Copy link
@vstinner

vstinner Jul 6, 2018

Author Member

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

This comment has been minimized.

Copy link
@JimJJewett

JimJJewett Jul 6, 2018

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

@ndevenish

This comment has been minimized.

Copy link

commented Jul 6, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

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

@vstinner

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2018

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:

This comment has been minimized.

Copy link
@alesdokshanin

alesdokshanin Jul 6, 2018

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

This comment has been minimized.

Copy link
@phord

phord Jul 6, 2018

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

This comment has been minimized.

Copy link
@Hultner

Hultner Jul 6, 2018

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

This comment has been minimized.

Copy link
@phord

phord Jul 6, 2018

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

This comment has been minimized.

Copy link
@phord

phord Jul 6, 2018

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.

This comment has been minimized.

Copy link
@Hultner

Hultner Jul 6, 2018

Yeah this was previously noted in #8095

This comment has been minimized.

Copy link
@motleytech

motleytech Aug 11, 2018

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

This comment has been minimized.

Copy link
@phord

phord Jul 6, 2018

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:

This comment has been minimized.

Copy link
@JimJJewett

JimJJewett Jul 6, 2018

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:

This comment has been minimized.

Copy link
@JimJJewett

JimJJewett Jul 6, 2018

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()):

This comment has been minimized.

Copy link
@JimJJewett

JimJJewett Jul 6, 2018

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

This comment has been minimized.

Copy link
@JimJJewett

JimJJewett Jul 6, 2018

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

This comment has been minimized.

Copy link
@JimJJewett

JimJJewett Jul 6, 2018

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.

This comment has been minimized.

Copy link
@amcgregor

amcgregor Feb 5, 2019

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)

This comment has been minimized.

Copy link
@JimJJewett

JimJJewett Jul 6, 2018

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

This comment has been minimized.

Copy link
@graingert

graingert Jul 6, 2018

Contributor

This doesn't even use the assignment expression

This comment has been minimized.

Copy link
@motleytech

motleytech Aug 11, 2018

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

@skrah skrah removed their request for review Jul 6, 2018

@vstinner

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

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.

@vstinner vstinner closed this Jul 12, 2018

@vstinner vstinner deleted the vstinner:wip_all_assign_expr branch Jul 12, 2018

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

This comment has been minimized.

Copy link
@motleytech

motleytech Aug 11, 2018

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

This comment has been minimized.

Copy link
@motleytech

motleytech Aug 11, 2018

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()))]

This comment has been minimized.

Copy link
@motleytech

motleytech Aug 11, 2018

Sweet refactor!

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

This comment has been minimized.

Copy link
@motleytech

motleytech Aug 11, 2018

== should be !=

@motleytech

This comment has been minimized.

Copy link

commented Aug 11, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.