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

Apply ruff/Perflint rule PERF401 #4449

Merged
merged 1 commit into from
Jun 29, 2024
Merged

Conversation

DimitriPapadopoulos
Copy link
Contributor

Summary of changes

PERF401 Use a list comprehension to create a transformed list

Pull Request Checklist

PERF401 Use a list comprehension to create a transformed list
@jaraco
Copy link
Member

jaraco commented Jun 29, 2024

I love love love this change. I've shifted my way of thinking on this since Python 2.0 introduced them. Glad to see it enforced here and everywhere. Please consider contributing the rule change to jaraco/skeleton, as I consider it to be a bug to use the init/for/append loop.

@jaraco jaraco merged commit 11b2f5f into pypa:main Jun 29, 2024
22 checks passed
@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jul 19, 2024

@jaraco Just FYI, other maintainers of Python libraries don't like complex list comprehensions as they appear to be less readable than loops. See for example:
numpy/numpy#26876 (comment)

After taking some time to think about it, I don't disagree. Readability seems more important than (marginal?) gains in performance. Perhaps PERF401 should not be enforced blindly.

@jaraco
Copy link
Member

jaraco commented Jul 19, 2024

For me, the issue isn't about performance or readability, but about logical and semantic safety.

I agree that readability suffers slightly with comprehensions and generator expressions due to the choices the Python language has made as to the syntax. It was one of the reasons I was initially reluctant to adopt them, even though my fallback was still based on a functional paradigm, using filter and map and reduce.

Aside from performance, the problem with a init/for/append loop is it requires multiple expressions and scopes to perform its work, scopes that are directly exposed to the programmer. Take that diff for example:

image

The imperative form allows for 6 different places where a user could alter alter the behavior in arbitrary ways (maybe just 5, since the first one applies in both scenarios). A user reviewing that code needs to mentally account for all the variability that can happen in that space, such as masking a variable or injecting some side-effect or mutating the list in unusual ways.

On the other hand, the dict comprehension, because it's a single expression, constrains the programmer and provides the reader (and the interpreter) with a lot of guarantees about the safety of the operation. It limits the side-effects and arbitrary logic. It very cleanly expresses a simple and common concept without having the programmer hand-encode that concept. It's for this reason that functional languages like LISP and Haskell are well-known for their safety and sophistication without compromise.

If we generalize the critique of the numpy maintainer, no one should ever use comprehensions. That's an untenable position to apply generally.

Personally, I would have written that block of code as

return functools.reduce(operator.or_, dicts)

or even re-define the function as:

merge_api_dicts = functools.partial(functools.reduce, functools.operator.or_)

Some would say that's even less readable, because you aren't walking the user through every minute detail, but that's the whole point of programming - to create increasingly sophisticated abstractions so you don't have to deal with the minutiae.

I'm delighted to have perf 401 and I respect that maybe others still prefer the C-style imperative approach, but for me and projects I maintain, I'd like to demonstrate and promote higher level abstractions, including comprehensions.

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

Successfully merging this pull request may close these issues.

2 participants