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

bpo-45975: Simplify some while-loops with walrus operator #29347

Merged
merged 10 commits into from
Nov 26, 2022

Conversation

nickdrozd
Copy link
Contributor

@nickdrozd nickdrozd commented Oct 31, 2021

The following pattern occurs a few times in the codebase:

while 1:
    data = conn.recv(blocksize)
    if not data:
        break
    ...

There is an unbounded while loop in which some kind of input is read. If the input is there, it is processed and the loop is continued; otherwise the loop is broken.

This can be expressed more cleanly with the walrus operator:

while data := conn.recv(blocksize):
   ...

Some of this code goes back to the days of Python 1. I assume that the original authors would have used the walrus idiom if it had been available, which it wasn't. But now it is.

Rewriting the instances of this pattern shaves off 148 lines from the codebase. Removing the manual break statements makes the logic more straightforward.

This should not change the behavior of anything. I'm assuming that this code is all under test and that any deviations from the existing behavior will be caught. Anything that isn't tested shouldn't be changed (and should be tested).

https://bugs.python.org/issue45975

@terryjreedy
Copy link
Member

A code change this extensive must have a bpo issue where the advisability of this change can be debated. I intentionally did not click the [Approve and run] pending that. The opening 'comment' above can be copied as the opening post. It is far too long for a merge comment but great as an issue opener. Connect this PR to the issue by adding 'bpo-#####' to the title. Some issues:

Backports: Code reformats are often not backported. In the other hand, I would want idlelib changes backported after reviewing them. Make a separate PR for idlelib changes. I will likely approve and merge it but have little opinion re other modules.

Deprecated modules (distutils) are generally not patched.

There might be other reasons to oppose changes other modules.

Copy link
Contributor

@jdevries3133 jdevries3133 left a comment

Choose a reason for hiding this comment

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

lgtm other than 1 potential bug. The changes definitely need to be discussed on the bpo, but it's a +1 from me!

Lib/sre_compile.py Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 2, 2021
@arhadthedev
Copy link
Member

You really need to:

  1. Create an issue on bpo (bugs.python.org)
  2. Look up its url (it looks like https://bugs.python.org/issue6778) and copy a number after /issue part
  3. Prepend a title of this pull request with bpo-NNNN: where NNNN is the copied number so the bot will link everything necessary.

The bpo is used since pre-Github times (Python repository migrated many times between hostings), so all development discussions go there.

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Why parentheses are used? They are not required as I see.

Lib/_pyio.py Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Dec 3, 2021
@nickdrozd nickdrozd changed the title Simplify some while-loops with walrus operator bpo-45975: Simplify some while-loops with walrus operator Dec 3, 2021
@iritkatriel
Copy link
Member

This will make backports to 3.6 and 3.7 more complicated.

@arhadthedev
Copy link
Member

@iritkatriel Are backports necessary here? This is neither a bugfix nor a new feature.

@iritkatriel
Copy link
Member

@iritkatriel Are backports necessary here? This is neither a bugfix nor a new feature.

No but if there will be a need in the future to backport a security fix in the same code, it won’t work cleanly because of this PR.

So that’s an argument against this sweeping change. What is the argument in its favour?

Lib/base64.py Outdated Show resolved Hide resolved
@nickdrozd
Copy link
Contributor Author

Rebased on main. Reverted changes to distutils (deprecated) and idlelib (split off to #31083).

@merwok
Copy link
Member

merwok commented Feb 3, 2022

For future PRs to CPython, please don’t use force pushes, they make poor experience for reviewers.
All PRs use squash merges so it doesn’t matter if branches are messy. Thanks!

@nickdrozd
Copy link
Contributor Author

I saw that aifc is among the "dead batteries" to be removed, so I undid the change there.

Lib/base64.py Outdated Show resolved Hide resolved
Lib/http/client.py Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@nickdrozd
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

@ethanfurman ethanfurman added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 29, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ethanfurman for commit 1583049 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 29, 2022
@nickdrozd
Copy link
Contributor Author

Hello, just checking in on this. I suppose it needs to be updated. I can do that, or someone else can.

@eendebakpt
Copy link
Contributor

Hello, just checking in on this. I suppose it needs to be updated. I can do that, or someone else can.

@nickdrozd Can you resolve the merge conflicts? I will then review.

@nickdrozd
Copy link
Contributor Author

@eendebakpt Updated!

@eendebakpt
Copy link
Contributor

PR looks good to me. The status is awaiting merge. I guess @ethanfurman will have to do that.

@ethanfurman ethanfurman merged commit 024ac54 into python:main Nov 26, 2022
@nickdrozd
Copy link
Contributor Author

Thanks to all the reviewers! Your work is greatly appreciated ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.