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

[perflint] - Fix manual-list-comprehension for async generators (PERF401) #14551

Closed
wants to merge 2 commits into from

Conversation

diceroll123
Copy link
Contributor

Summary

Fixes a current bug in PERF401's preview autofix, where it tries to make an async generator within a list.extend, without wrapping the generator in a list, which does not implement __iter__ which makes it an error.

Test Plan

cargo test

Copy link
Contributor

github-actions bot commented Nov 23, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@diceroll123
Copy link
Contributor Author

Found a weird issue with comments, this was already present in the autofix in 0.8.0, but I decided to fix it and put it in here too.

@Skylion007
Copy link
Contributor

@w0nder1ng Another edge case. May want to merge this PR with #14369

@diceroll123
Copy link
Contributor Author

Hm, yeah, my implementation does not do well with this one:

def f():
    # make sure that `tmp` is not deleted
    tmp = 1; result = [] # commment should be protected
    for i in range(10):
        result.append(i + 1) # PERF401

@dylwil3 dylwil3 added bug Something isn't working fixes Related to suggested fixes for violations labels Nov 26, 2024
@MichaReiser
Copy link
Member

@diceroll123 #14369 is now merged. Would you mind rebasing your PR?

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Needs rebase

@diceroll123 diceroll123 force-pushed the fix-async-for-perf401 branch from d5523a0 to aafe95f Compare January 15, 2025 09:08
@diceroll123
Copy link
Contributor Author

Woop, destroyed the commit here by accident, but after fixing it locally and coming back from rebase hell, it turns out all of the issues my PR solved were solved by 14369, so I'll close this. Thanks all! 😄

@MichaReiser
Copy link
Member

MichaReiser commented Jan 15, 2025

Thank you and sorry for the rebase struggles

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants