Skip to content

Convert several for loops in dataclasses module to list comprehensions #114011

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
keithasaurus opened this issue Jan 13, 2024 · 2 comments
Closed
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@keithasaurus
Copy link
Contributor

keithasaurus commented Jan 13, 2024

Feature or enhancement

Proposal:

There are several for loops in the dataclasses module where performance would presumably improve with list comprehensions. Arguably the code becomes a bit clearer as well.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

@keithasaurus keithasaurus added the type-feature A feature request or enhancement label Jan 13, 2024
@keithasaurus keithasaurus changed the title Convert several for loops in data classes to list comprehensions Convert several for loops in dataclasses module to list comprehensions Jan 13, 2024
@carljm
Copy link
Member

carljm commented Jan 13, 2024

It's rarely advisable to presume that something will improve performance without benchmarking it :) In all Python versions prior to 3.12, it's likely that if anything this conversion would have been a performance regression. In 3.12+ I expect it shouldn't be a regression anymore, due to comprehension inlining, but I doubt it will be a meaningful improvement, either. Without benchmarks, we should not consider performance as a factor.

I do personally find the code a bit nicer with the comprehensions, but we don't usually take PRs for such minor and subjective code changes, just due to the inherent risk of any change.

@AlexWaygood AlexWaygood added the stdlib Python modules in the Lib dir label Jan 13, 2024
@terryjreedy
Copy link
Member

Yesterday I, as main idlelib maintainer, replaced 6 awkward lines in an IDLE test method, including a nested function, with a much clearer one-line dict comprehension. If I were the main dataclass maintainer, I would likely merge the two return changes, maybe with a format tweak, as they improve the parallelism of the if-else returns. But think the current dataclass module experts, @ericvsmith and @carljm should decide this one. I might check whether the affected functions are covered by tests.

@AlexWaygood AlexWaygood closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants