Skip to content

gh-114011: Convert for loops in dataclasses module to comprehensions #114012

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

Closed

Conversation

keithasaurus
Copy link
Contributor

gh-114011: Convert for loops in dataclasses module to comprehensions

This is a simple porting of for loops to list comprehensions for performance and readability. I haven't done benchmarking but can if needed.

@bedevere-app
Copy link

bedevere-app bot commented Jan 13, 2024

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.

@AlexWaygood AlexWaygood changed the title Convert for loops in dataclasses module to comprehensions gh-114011: Convert for loops in dataclasses module to comprehensions Jan 13, 2024
Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thank you, but CPython typically does not accept patches that are primarily cosmetic. If this is for performance reasons, benchmarking the change would be great!

@ericvsmith ericvsmith self-assigned this Jan 13, 2024
@rhettinger
Copy link
Contributor

Stylistically, I don't think these particular changes should be made.

Usually, we prefer list comprehensions when they are short and sweet. But when the bodies grow larger, it is almost always more readable and maintainable to have a traditional loop. The particular changes in the PR are too large to mentally consume in one gulp.

@keithasaurus
Copy link
Contributor Author

@rhettinger

Two of the three changes here deal with the _asdict_inner function. Ok if I re-target toward optimizing that function? The second two list comprehensions would be part of that, but neither of are particularly difficult on the eyes -- no if or walrus operator. There are several other aspects of that function that I think could be modified to speed it up as well -- for example, not checking if something is an instance of a tuple twice, which it currently does. I'm guessing there's something like a 5% speedup available with modest changes.

@keithasaurus
Copy link
Contributor Author

Will open a separate PR for speeding up dataclasses.asdict

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.

4 participants