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

Make SIM401 catch ternary operations #7415

Merged

Conversation

Flowrey
Copy link
Contributor

@Flowrey Flowrey commented Sep 15, 2023

Summary

Fixes #7288
Make SIM401 rules to catch ternary operations.

Test Plan

Tested against SIM401.py fixtures

@Flowrey Flowrey force-pushed the make-SIM401-catch-ternay-operations branch 3 times, most recently from 6c53ca1 to da38373 Compare September 15, 2023 19:13
@Flowrey Flowrey force-pushed the make-SIM401-catch-ternay-operations branch from da38373 to 720d476 Compare September 15, 2023 19:32
@Flowrey Flowrey marked this pull request as ready for review September 15, 2023 19:39
@github-actions
Copy link

github-actions bot commented Sep 15, 2023

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+3, -1, 0 error(s))

rotki (+3, -1)

- [*] 11 fixable with the `--fix` option (223 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ [*] 11 fixable with the `--fix` option (225 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ rotkehlchen/db/updates.py:204:27: SIM401 Use `rule_data.get('links', {})` instead of an `if` block
+ rotkehlchen/externalapis/cryptocompare.py:327:24: SIM401 Use `json_ret.get('Data', json_ret)` instead of an `if` block

Rules changed: 1
Rule Changes Additions Removals
SIM401 2 2 0

@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label Sep 18, 2023
Comment on lines 1033 to 1041
let node = orelse.clone();
let node1 = *test_key.clone();
let node2 = ast::ExprAttribute {
value: expected_subscript.clone(),
attr: Identifier::new("get".to_string(), TextRange::default()),
ctx: ExprContext::Load,
range: TextRange::default(),
};
let node3 = ast::ExprCall {
Copy link
Member

Choose a reason for hiding this comment

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

nit: could you give them more descriptive names than node 1/2/3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure !

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 18, 2023

CodSpeed Performance Report

Merging #7415 will not alter performance

Comparing Flowrey:make-SIM401-catch-ternay-operations (a2ac4b7) with main (7a5f988)

Summary

✅ 25 untouched benchmarks

@Flowrey Flowrey force-pushed the make-SIM401-catch-ternay-operations branch from a070c74 to 2334c66 Compare September 18, 2023 18:15
@charliermarsh
Copy link
Member

My only hesitation here is whether this should be a separate rule and / or whether it should be behind the preview flag.

@charliermarsh charliermarsh self-assigned this Sep 21, 2023
@Flowrey
Copy link
Contributor Author

Flowrey commented Sep 23, 2023

I've looked at flake8-simplify and they dont seem to implement SIM401 for ternary operators.

@charliermarsh
Copy link
Member

I'll take this one to completion -- gonna put this behavior under preview mode.

@charliermarsh charliermarsh enabled auto-merge (squash) October 20, 2023 20:38
@charliermarsh charliermarsh merged commit fa556d1 into astral-sh:main Oct 20, 2023
15 checks passed
@zanieb zanieb added the preview Related to preview mode features label Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIM401 should catch ternary operations
5 participants