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-40077: Convert itertools types to heap types #24065

Closed

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 2, 2021

@erlend-aasland
Copy link
Contributor Author

@rhettinger, let me know how large chunks you want me to split this PR in.

@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 4, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 55cee70 🤖

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 Jan 4, 2021
@rhettinger
Copy link
Contributor

  • I can't review this because I don't fully understand the changes
  • I'm unclear why this is necessary.
  • The code churn is enormous.
  • There are measurable performance hits.
  • There are no doc changes or tests, so I'm unclear what the user benefit would be.

@erlend-aasland
Copy link
Contributor Author

  • I can't review this because I don't fully understand the changes

The changes are described in the PR subject: "Convert itertools types to heap types"
As an improvement, and in order to make the change more explicit, I could alter the subject to "Convert itertools types to heap allocated types".

In order to fully understand the changes, I would recommend taking a look at PEP 384, PEP 573, PyType_Spec, and PyType_Slot.

I guess @vstinner or @corona10 could assist in reviewing this, if they don't mind.

  • I'm unclear why this is necessary.
  • The code churn is enormous.

#24065 (comment)

  • There are measurable performance hits.

https://mail.python.org/archives/list/python-dev@python.org/thread/S5GZZCEREZLA2PEMTVFBCDM52H4JSENR/#RIK75U3ROEHWZL4VENQSQECB4F4GDELV

  • There are no doc changes

The itertools docs does not contain information about how its types are implemented. I see no reason to add this information.

or tests

A module should not alter its behaviour depending on how its types are implemented. I see no reason to add type implementation specific tests for itertools.

, so I'm unclear what the user benefit would be.

@vstinner
Copy link
Member

vstinner commented Jan 6, 2021

I prefer to close the PR for now. I'm working on a PEP with @corona10 to explain the rationale. We can wait until this (future) PEP is accepted before changing itertools.

@vstinner vstinner closed this Jan 6, 2021
@erlend-aasland erlend-aasland deleted the bpo-40077/itertools branch January 6, 2021 11:41
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 6, 2021

I prefer to close the PR for now. I'm working on a PEP with @corona10 to explain the rationale. We can wait until this (future) PEP is accepted before changing itertools.

All right!

I'll keep the branch around, then :)

@erlend-aasland
Copy link
Contributor Author

FYI, PEP-687 was just accepted.

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.

7 participants