Add flake8-simplify SIM300 check for Yoda Conditions #1539

PedramNavid · 2023-01-01T22:15:17Z

Adds a SIM300 Check for Yoda Conditions

resources/test/fixtures/flake8_simplify/SIM300.py:3:1: SIM300 Use `'yoda' == compare` instead of `compare == 'yoda' (Yoda-conditions)`
resources/test/fixtures/flake8_simplify/SIM300.py:4:1: SIM300 Use `42 == age` instead of `age == 42 (Yoda-conditions)`
Found 2 error(s).

Refs: #998

charliermarsh

This looks great! Thank you for putting it together!

Couple small things -- only behavior change would be catching the constant-constant case, if you agree.

charliermarsh · 2023-01-01T22:22:35Z

resources/test/fixtures/flake8_simplify/SIM300.py

+# Bad
+
+'yoda' == compare # SIM300
+42 == age # SIM300


One condition that flake8-simplify seems to get wrong:

"yoda" == "yoda"

flake8-simplify gives me:

resources/test/fixtures/flake8_simplify/SIM300.py:10:1: SIM300 Use '"yoda" == "yoda"' instead of '"yoda" == "yoda"' (Yoda-conditions)

But we should probably avoid raising this in the event that both sides are constant?

Oh good catch, I'll add another check. Might be worth creating an alert at some point when comparing constants with each other too. Can't think of any good reason to do it on purpose.

Haha yeah the absurdity of that didn't even occur to me... I have my correctness hat on.

charliermarsh · 2023-01-01T22:23:06Z

resources/test/fixtures/flake8_simplify/SIM300.py

@@ -0,0 +1,9 @@
+# Bad
+
+'yoda' == compare # SIM300


Couple other good test cases:

x == y and 'yoda' == compare (should raise)

'yoda' == compare == 1 (shouldn't raise)

charliermarsh · 2023-01-01T22:23:44Z

src/flake8_simplify/plugins/yoda_conditions.rs

+use crate::checkers::ast::Checker;
+use crate::checks::{Check, CheckKind};
+
+pub fn yoda_conditions(


Mind just adding /// SIM300 above this function? Helps a lot with grepping for rule implementations, since otherwise there's nothing tying this file to the check code.

charliermarsh · 2023-01-01T22:24:26Z

CONTRIBUTING.md

-#[test_case(CheckCode::A001, Path::new("A001.py"); "A001")]
-...
-```
+Run Ruff locally with (e.g.) `cargo run resources/test/fixtures/E402.py --no-cache --select E402`.


I think this line might be repeated between here and the previous paragraph.

charliermarsh · 2023-01-01T23:37:54Z

Awesome, thank you!

charliermarsh · 2023-01-01T23:38:54Z

If you were interested in enabling autofix for this rule, you could take a look at the compare function in src/pycodestyle/plugins.rs, which demonstrates the use of SourceCodeGenerator to take a modified AST and generate Python source code from it.

PedramNavid · 2023-01-02T00:19:15Z

If you were interested in enabling autofix for this rule, you could take a look at the compare function in src/pycodestyle/plugins.rs, which demonstrates the use of SourceCodeGenerator to take a modified AST and generate Python source code from it.

great next step. thanks for the hint i'll have a look!

[![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://togithub.com/charliermarsh/ruff) | `^0.0.206` -> `^0.0.207` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.207/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.207/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.207/compatibility-slim/0.0.206)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.207/confidence-slim/0.0.206)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.207`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.207) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.206...v0.0.207) #### What's Changed - Implement list-to-tuple comprehension unpacking by [@colin99d](https://togithub.com/colin99d) in [astral-sh/ruff#1534 - Avoid triggering PD errors on method calls by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1537 - Avoid PD false positives on some non-DataFrame expressions by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1538 - Correct UP027 message to “generator expression” by [@andersk](https://togithub.com/andersk) in [astral-sh/ruff#1540 - Add flake8-simplify SIM300 check for Yoda Conditions by [@PedramNavid](https://togithub.com/PedramNavid) in [astral-sh/ruff#1539 - Print warning when running debug builds without --no-cache by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1549 - Fix typing::match_annotated_subscript matching ExprKind::Call by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1554 - Add clarification by [@VictorGob](https://togithub.com/VictorGob) in [astral-sh/ruff#1557 - Add explicit new-rule recommendation in CONTRIBUTING.md by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1558 - Detect unpacking assignments in eradicate by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#1559 - Fix `__init__.py` being private by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#1556 #### New Contributors - [@PedramNavid](https://togithub.com/PedramNavid) made their first contribution in [astral-sh/ruff#1539 - [@VictorGob](https://togithub.com/VictorGob) made their first contribution in [astral-sh/ruff#1557 **Full Changelog**: astral-sh/ruff@v0.0.206...v0.0.207 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] 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://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).  Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

BenediktAllendorf · 2023-01-04T16:49:53Z

I was just writing an issue because I thought this check wanted me to use Yoda conditions, and thus it all didn't make sense to me. But now I think this is more of a communication thing, and I hope it's okay to address that here.

First, I'm not sure if the description Use left == right instead of right == left (Yoda-conditions) is the best way to clarify what your code is generally supposed to look like according to this check.

Second, Use `42 == age` instead of `age == 42 (Yoda-conditions)` (besides having the last tick at the wrong location) sounds like I should use that format (i.e., I should use Yoda-conditions), but I think the message is supposed to mean "you are wrongly doing X instead of correctly doing Y"?

Maybe the message could be more along the line of:
Usage of Yoda-conditions is discouraged; use `age == 42` instead?

charliermarsh · 2023-01-04T17:08:41Z

Yeah this message was taken directly from flake8-simplify for simplicity (sorry for the pun). I'll tweak it a bit, I agree that it's suboptimal.

charliermarsh · 2023-01-04T21:05:34Z

(Fixed.)

BenediktAllendorf · 2023-01-05T07:40:02Z

Thank you for your quick response! I'm very impressed by this tool and am looking forward to integrating it into our project(s) more and more!

PedramNavid added 2 commits January 1, 2023 14:13

Add flake8-simplify SIM300 check for Yoda Conditions

9b13bb3

Update Contribution instructions and add SIM300 to README

bff55b3

charliermarsh reviewed Jan 1, 2023

View reviewed changes

PedramNavid and others added 4 commits January 1, 2023 14:58

Don't alert when comparing a const to a const.

f66ff0e

Update Contributing

c6c1bd2

Merge branch 'main' into SIM300

b73dc11

Tweak test fixture

d38e8e0

charliermarsh merged commit 07e47be into astral-sh:main Jan 1, 2023

PedramNavid deleted the SIM300 branch January 2, 2023 00:19

rabelux mentioned this pull request Jan 18, 2023

Possible overlap between SIM300 and PLC2201 #1954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flake8-simplify SIM300 check for Yoda Conditions #1539

Add flake8-simplify SIM300 check for Yoda Conditions #1539

PedramNavid commented Jan 1, 2023 •

edited

Loading

charliermarsh left a comment

charliermarsh Jan 1, 2023

PedramNavid Jan 1, 2023

charliermarsh Jan 1, 2023

charliermarsh Jan 1, 2023

charliermarsh Jan 1, 2023

charliermarsh Jan 1, 2023

charliermarsh commented Jan 1, 2023

charliermarsh commented Jan 1, 2023 •

edited

Loading

PedramNavid commented Jan 2, 2023

BenediktAllendorf commented Jan 4, 2023

charliermarsh commented Jan 4, 2023

charliermarsh commented Jan 4, 2023

BenediktAllendorf commented Jan 5, 2023

Add flake8-simplify SIM300 check for Yoda Conditions #1539

Add flake8-simplify SIM300 check for Yoda Conditions #1539

Conversation

PedramNavid commented Jan 1, 2023 • edited Loading

charliermarsh left a comment

Choose a reason for hiding this comment

charliermarsh Jan 1, 2023

Choose a reason for hiding this comment

PedramNavid Jan 1, 2023

Choose a reason for hiding this comment

charliermarsh Jan 1, 2023

Choose a reason for hiding this comment

charliermarsh Jan 1, 2023

Choose a reason for hiding this comment

charliermarsh Jan 1, 2023

Choose a reason for hiding this comment

charliermarsh Jan 1, 2023

Choose a reason for hiding this comment

charliermarsh commented Jan 1, 2023

charliermarsh commented Jan 1, 2023 • edited Loading

PedramNavid commented Jan 2, 2023

BenediktAllendorf commented Jan 4, 2023

charliermarsh commented Jan 4, 2023

charliermarsh commented Jan 4, 2023

BenediktAllendorf commented Jan 5, 2023

PedramNavid commented Jan 1, 2023 •

edited

Loading

charliermarsh commented Jan 1, 2023 •

edited

Loading