-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[refurb] Implement reimplemented-starmap
rule (FURB140
)
#7253
[refurb] Implement reimplemented-starmap
rule (FURB140
)
#7253
Conversation
1ab6401
to
0f72b35
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
} | ||
|
||
// FURB140 | ||
pub(crate) fn reimplemented_starmap<T: StarmapEquivalent + Ranged>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of a generic here means that Rust generates a dedicated implementation for each T
instantiation.
I think it may be better to define a AnyComprehension
enum (or similar) with a variant for each node that you want to support instead of using a trait. See
ruff/crates/ruff_python_formatter/src/expression/expr_yield.rs
Lines 10 to 35 in c05e462
pub(super) enum AnyExpressionYield<'a> { | |
Yield(&'a ExprYield), | |
YieldFrom(&'a ExprYieldFrom), | |
} | |
impl<'a> AnyExpressionYield<'a> { | |
const fn is_yield_from(&self) -> bool { | |
matches!(self, AnyExpressionYield::YieldFrom(_)) | |
} | |
fn value(&self) -> Option<&Expr> { | |
match self { | |
AnyExpressionYield::Yield(yld) => yld.value.as_deref(), | |
AnyExpressionYield::YieldFrom(yld) => Some(&yld.value), | |
} | |
} | |
} | |
impl Ranged for AnyExpressionYield<'_> { | |
fn range(&self) -> TextRange { | |
match self { | |
AnyExpressionYield::Yield(yld) => yld.range(), | |
AnyExpressionYield::YieldFrom(yld) => yld.range(), | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of such a solution. With the trait, we don't need to perform any runtime checks for the given type. With the enum, we do them 4 times: element
, generators
, range
, and try_fix
. These seem not critical in any way, but one can argue that bloating from instantiating the same (rather small) generic 3 times is kind of similar.
I guess I can see how an enum solution is better for decoupling, if I remove try_fix
from that interface altogether. Then it can be used in other places that share comprehensions. On the other hand, this interface can't include DictComprehension
without making element
optional and adding more runtime checks.
Sorry, for the rant. If you insist, I can change it, but I hope you see my point here 😄
I have a few coding nits. Leaving it to @charliermarsh to review the rule logic |
0f72b35
to
4ec3f0b
Compare
CodSpeed Performance ReportMerging #7253 will degrade performances by 2.38%Comparing Summary
Benchmarks breakdown
|
d014c7c
to
ea69b1c
Compare
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://docs.astral.sh/ruff) ([source](https://togithub.com/astral-sh/ruff), [changelog](https://togithub.com/astral-sh/ruff/releases)) | `==0.0.290` -> `==0.0.291` | [![age](https://developer.mend.io/api/mc/badges/age/pypi/ruff/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/ruff/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/ruff/0.0.290/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/ruff/0.0.290/0.0.291?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>astral-sh/ruff (ruff)</summary> ### [`v0.0.291`](https://togithub.com/astral-sh/ruff/releases/tag/v0.0.291) [Compare Source](https://togithub.com/astral-sh/ruff/compare/v0.0.290...v0.0.291) <!-- Release notes generated using configuration in .github/release.yml at v0.0.291 --> #### What's Changed ##### Deprecations **The `format` command-line argument and configuration option has been renamed to `output-format`.** While Ruff will continue to respect `format` when passed as a command-line argument or configuration option, this backwards-compatible support will be dropped in a future release. See: [https://github.com/astral-sh/ruff/pull/7514](https://togithub.com/astral-sh/ruff/pull/7514). ##### Rules - \[`flake8-bandit`] Implement `S201`: `flask-debug-true` by [@​mkniewallner](https://togithub.com/mkniewallner) in [https://github.com/astral-sh/ruff/pull/7503](https://togithub.com/astral-sh/ruff/pull/7503) - \[`flake8-bandit`] Implement `S507`: `ssh_no_host_key_verification` by [@​mkniewallner](https://togithub.com/mkniewallner) in [https://github.com/astral-sh/ruff/pull/7528](https://togithub.com/astral-sh/ruff/pull/7528) - \[`flake8-logging`] Implement `LOG002`: `invalid-get-logger-argument` by [@​dhruvmanila](https://togithub.com/dhruvmanila) in [https://github.com/astral-sh/ruff/pull/7399](https://togithub.com/astral-sh/ruff/pull/7399) - \[`flake8-logging`] Implement `LOG007`: `exception-without-exc-info` by [@​qdegraaf](https://togithub.com/qdegraaf) in [https://github.com/astral-sh/ruff/pull/7410](https://togithub.com/astral-sh/ruff/pull/7410) - \[`refurb`] Implement `FURB140`: `reimplemented-starmap` by [@​SavchenkoValeriy](https://togithub.com/SavchenkoValeriy) in [https://github.com/astral-sh/ruff/pull/7253](https://togithub.com/astral-sh/ruff/pull/7253) - \[`refurb`] Implement `FURB148`: `unnecessary-enumerate` by [@​tjkuson](https://togithub.com/tjkuson) in [https://github.com/astral-sh/ruff/pull/7454](https://togithub.com/astral-sh/ruff/pull/7454) - \[`ruff`] Detect `asyncio.get_running_loop` calls in RUF006 by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7562](https://togithub.com/astral-sh/ruff/pull/7562) ##### Settings - Show `--no-X` variants in CLI help by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7504](https://togithub.com/astral-sh/ruff/pull/7504) - Rename `format` option to `output-format` by [@​MichaReiser](https://togithub.com/MichaReiser) in [https://github.com/astral-sh/ruff/pull/7514](https://togithub.com/astral-sh/ruff/pull/7514) - Enable tab completion for `ruff rule` by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7560](https://togithub.com/astral-sh/ruff/pull/7560) ##### Bug Fixes - Add padding to prevent some autofix errors by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7461](https://togithub.com/astral-sh/ruff/pull/7461) - Remove parentheses when rewriting assert calls to statements by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7464](https://togithub.com/astral-sh/ruff/pull/7464) - Avoid flagging starred elements in C402 by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7466](https://togithub.com/astral-sh/ruff/pull/7466) - Extend `bad-dunder-method-name` to permit `attrs` dunders by [@​tjkuson](https://togithub.com/tjkuson) in [https://github.com/astral-sh/ruff/pull/7472](https://togithub.com/astral-sh/ruff/pull/7472) - Avoid N802 violations for [@​overload](https://togithub.com/overload) methods by [@​JonathanPlasse](https://togithub.com/JonathanPlasse) in [https://github.com/astral-sh/ruff/pull/7498](https://togithub.com/astral-sh/ruff/pull/7498) - Avoid flagging starred expressions in UP007 by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7505](https://togithub.com/astral-sh/ruff/pull/7505) - Ensure that LOG007 only triggers on `.exception()` calls by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7524](https://togithub.com/astral-sh/ruff/pull/7524) - Use strict sorted and union for NoQA mapping insertion by [@​dhruvmanila](https://togithub.com/dhruvmanila) in [https://github.com/astral-sh/ruff/pull/7531](https://togithub.com/astral-sh/ruff/pull/7531) - Avoid inserting imports directly after continuation by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7553](https://togithub.com/astral-sh/ruff/pull/7553) - Add padding in `PERF102` fixes by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7554](https://togithub.com/astral-sh/ruff/pull/7554) - Avoid invalid fix for parenthesized values in F601 by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7559](https://togithub.com/astral-sh/ruff/pull/7559) - Treat `os.error` as an `OSError` alias by [@​charliermarsh](https://togithub.com/charliermarsh) in [https://github.com/astral-sh/ruff/pull/7582](https://togithub.com/astral-sh/ruff/pull/7582) - Extend `bad-dunder-method-name` to permit `__html__` by [@​jaap3](https://togithub.com/jaap3) in [https://github.com/astral-sh/ruff/pull/7492](https://togithub.com/astral-sh/ruff/pull/7492) - Fix stylist indentation with a formfeed by [@​konstin](https://togithub.com/konstin) in [https://github.com/astral-sh/ruff/pull/7489](https://togithub.com/astral-sh/ruff/pull/7489) #### New Contributors - [@​MicaelJarniac](https://togithub.com/MicaelJarniac) made their first contribution in [https://github.com/astral-sh/ruff/pull/5498](https://togithub.com/astral-sh/ruff/pull/5498) - [@​maheshsaripalli9](https://togithub.com/maheshsaripalli9) made their first contribution in [https://github.com/astral-sh/ruff/pull/7552](https://togithub.com/astral-sh/ruff/pull/7552) - [@​T-256](https://togithub.com/T-256) made their first contribution in [https://github.com/astral-sh/ruff/pull/7585](https://togithub.com/astral-sh/ruff/pull/7585) **Full Changelog**: astral-sh/ruff@v0.0.290...v0.0.291 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/allenporter/flux-local). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi45Ny4xIiwidXBkYXRlZEluVmVyIjoiMzYuOTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Summary
This PR is part of a bigger effort of re-implementing
refurb
rules #1348. It adds support for FURB140Test Plan
I included a new test + checked that all other tests pass.