GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-126317: Simplify stdlib code by using itertools.batched() by dongwooklee96 · Pull Request #126323 · python/cpython

Conversation

@dongwooklee96

@dongwooklee96 dongwooklee96 commented Nov 2, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Simplify stdlib code by using itertools.batched() #126317

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JSYoo5B and furkanonder reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@ghost

ghost commented Nov 2, 2024 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Nov 2, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting review label Nov 2, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Nov 2, 2024
Simplify stdlib code by using itertools.batched() #126317
Closed
@corona10 corona10 added the skip news label Nov 2, 2024
@corona10 corona10 assigned serhiy-storchaka Nov 2, 2024
@corona10

corona10 commented Nov 2, 2024

Copy link
Copy Markdown
Member

@serhiy-storchaka

@dongwooklee96 is the first-time contributor to the PyCon KR sprint today.
I am not sure if this change is worth accepting from my side.
So I am delegating the review to you :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Nov 2, 2024
View reviewed changes
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@dongwooklee96
pythongh-126317: Simplify stdlib code by using itertools.batched()
4fa459b
@dongwooklee96
dongwooklee96 force-pushed the gh-126317 branch from 11eb1ee to 4fa459b Compare November 2, 2024 11:02
@nineteendo

nineteendo commented Nov 2, 2024

Copy link
Copy Markdown
Contributor

dongwooklee96, could you avoid force pushing? https://devguide.python.org/getting-started/pull-request-lifecycle:

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.
dongwooklee96 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nineteendo
nineteendo reviewed Nov 2, 2024
View reviewed changes
Comment thread Lib/pickle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
nineteendo
nineteendo reviewed Nov 2, 2024
View reviewed changes
Comment thread Lib/pickle.py
tmp = list(islice(it, self._BATCHSIZE))
n = len(tmp)
if n > 1:
for batch in batched(items, self._BATCHSIZE):

@nineteendo nineteendo Nov 2, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is it worth using tmp instead of batch? It reduces the diff, but feel free to disregard.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@picnixz picnixz Nov 2, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think it's better to have a named variable that says what it is. "tmp" is too generic IMO.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@dongwooklee96 dongwooklee96 Nov 2, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I also think chunk would be better, should we change it to chunk?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@picnixz picnixz Nov 2, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Since we use "batchsize" I think it's fine to keep "batch"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/pickle.py
if len(batch) != 1:
write(MARK)
for i, x in enumerate(tmp, start):
for i, x in enumerate(batch, start):

@nineteendo nineteendo Nov 2, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is it faster to calculate start + i in case of an error?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@picnixz picnixz Nov 2, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's probably faster because we don't need to materialize an enumerate object but I'd benchmark that one to be sure.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dongwooklee96
pythongh-126317: Fix to avoid calling the len function twice
21d7b01
@dongwooklee96

dongwooklee96 commented Nov 2, 2024

Copy link
Copy Markdown
Contributor Author

dongwooklee96, could you avoid force pushing? https://devguide.python.org/getting-started/pull-request-lifecycle:

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

Sure, I understand. I won’t force-push from now on.
nineteendo reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Nov 2, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Nov 2, 2024
@serhiy-storchaka
serhiy-storchaka merged commit bd4be5e into python:main Nov 2, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 2, 2024
picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
@dongwooklee96 @picnixz
pythongh-126317: Simplify pickle code by using itertools.batched() (p…
6c5227d 
…ythonGH-126323)
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@dongwooklee96 @ebonnal
pythongh-126317: Simplify pickle code by using itertools.batched() (p…
96db8d0 
…ythonGH-126323)
@serhiy-storchaka serhiy-storchaka removed their assignment Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
+1 more reviewer
@nineteendo nineteendo nineteendo left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@dongwooklee96 @corona10 @nineteendo @serhiy-storchaka @picnixz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.