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

[flake8_comprehensions] Handled special case for C400 which also matches C416 #10419

Merged

Conversation

boolean-light
Copy link
Contributor

Summary

Short-circuit implementation mentioned in #10403.

I implemented this by extending C400:

  • Made UnnecessaryGeneratorList have information of whether the the short-circuiting occurred (to put diagnostic)
  • Add additional check for whether in unnecessary_generator_list function.

Please give me suggestions if you think this isn't the best way to handle this :)

Test Plan

Extended C400.py a little, and written the cases where:

  • Code could be converted to one single conversion to list e.g. list(x for x in range(3)) -> list(range(3))
  • Code couldn't be converted to one single conversion to list e.g. list(2 * x for x in range(3)) -> [2 * x for x in range(3)]
  • list function is not built-in, and should not modify the code in any way.

Copy link
Contributor

github-actions bot commented Mar 15, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh self-requested a review March 15, 2024 13:56
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Mar 15, 2024
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

return;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I decided to just use more nesting here rather than the break pattern. It's not necessarily "better", just a matter of personal preference.

);
let Some(ExprGenerator {
elt, generators, ..
}) = argument.as_generator_expr()
Copy link
Member

Choose a reason for hiding this comment

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

.as_generator_expr() lets us avoid the clone.

@charliermarsh charliermarsh enabled auto-merge (squash) March 15, 2024 14:22
@charliermarsh charliermarsh force-pushed the unnecessary-generator-list branch from 856331e to 1ca7107 Compare March 15, 2024 14:25
@charliermarsh charliermarsh merged commit 7e652e8 into astral-sh:main Mar 15, 2024
17 checks passed
charliermarsh pushed a commit that referenced this pull request Mar 26, 2024
…matches `C416` (#10596)

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

Similar to #10419, there was a case where there is a collision of C401
and C416 (as discussed in #10101).
Fixed this by implementing short-circuit for the comprehension of the
form `{x for x in foo}`.

## Test Plan

<!-- How was it tested? -->

Extended `C401.py` with the case where `set` is not builtin function,
and divided the case where the short-circuit should occur.
Removed the last testcase of `print(f"{ {set(a for a in 'abc')} }")`
test as this is invalid as a python code, but should I keep this?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants