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

[pylint] Implement consider-using-ternary (R1706) #7811

Merged
merged 41 commits into from
Oct 13, 2023

Conversation

danbi2990
Copy link
Contributor

@danbi2990 danbi2990 commented Oct 4, 2023

This is my first PR. Please feel free to give me any feedback for even small drawbacks.

Summary

Checks if pre-python 2.5 ternary syntax is used.

Before

x, y = 1, 2
maximum = x >= y and x or y  # [consider-using-ternary]

After

x, y = 1, 2
maximum = x if x >= y else y

References:
pylint
#970
and_or_ternary distinction logic

Test Plan

Unit test, python file, snapshot added.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 4, 2023

CodSpeed Performance Report

Merging #7811 will not alter performance

Comparing danbi2990:consider-using-ternary (2551cca) with main (6f9c317)

Summary

✅ 25 untouched benchmarks

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2023

PR Check Results

Ecosystem

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

rotki (+2, -1)

- [*] 11 fixable with the `--fix` option (20 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ [*] 11 fixable with the `--fix` option (21 hidden fixes can be enabled with the `--unsafe-fixes` option).
+ rotkehlchen/tests/fixtures/accounting.py:56:13: PLR1706 Consider using if-else expression

Rules changed: 1
Rule Changes Additions Removals
PLR1706 1 1 0

@danbi2990 danbi2990 marked this pull request as draft October 5, 2023 00:49
@danbi2990 danbi2990 marked this pull request as ready for review October 5, 2023 07:17
crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
@danbi2990
Copy link
Contributor Author

@konstin
All comments applied. Thank you!

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, i have added some more notes for the next iteration.

I've also looked through the ecosystem checks; I think we shouldn't warn when the parent is an if statement (e.g. https://github.com/apache/airflow/blob/609eed90698aa7abeb5bae9ae156062d32baae86/airflow/models/dag.py#L2639 or https://github.com/rotki/rotki/blob/7e88ac61f98e21e231b43186a6fbee2e090b1be5/package.py#L1070) or when the parent is a binary expression (e.g. https://github.com/bokeh/bokeh/blob/cf57b9ae0073ecf0434a8c10002a70f6bf2079cd/src/bokeh/core/property/dataspec.py#L588).

This line is also not a case of R1706, but the original code looks wrong, i'm pretty sure they meant

x.is_dir() and ((x / 'rotkehlchen.db').exists() or (x / 'rotkehlchen_transient.db').exists())

instead of

x.is_dir() and (x / 'rotkehlchen.db').exists() or (x / 'rotkehlchen_transient.db').exists()

, so imo that's fine.

crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/rules/pylint/helpers.rs Outdated Show resolved Hide resolved
@danbi2990
Copy link
Contributor Author

@konstin
All comments applied. Thank you. 🙇‍♂️

@konstin
Copy link
Member

konstin commented Oct 11, 2023

I found a case where the fix doesn't work (https://github.com/apache/airflow/blob/8fdf3582c2967161dd794f7efb53691d092f0ce6/airflow/utils/setup_teardown.py#L199):

new_list = [
    x
    for sublist in all_lists
    for x in sublist
    if (isinstance(operator, list) and x in operator) or x != operator
]

is fixed to

new_list = [
    x
    for sublist in all_lists
    for x in sublist
    if x in operator if isinstance(operator, list) else x != operator
]

which is invalid syntax, the expression needs parentheses here. @charliermarsh Do we have a util to detect whether we need to add parentheses when converting a bool op to a ternary expression?

@danbi2990
Copy link
Contributor Author

@konstin
How about this version? 🤔

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Looks good!

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Oct 13, 2023
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.

Thank you!

@charliermarsh charliermarsh enabled auto-merge (squash) October 13, 2023 01:21
@charliermarsh charliermarsh merged commit c03a693 into astral-sh:main Oct 13, 2023
15 checks passed
@danbi2990 danbi2990 deleted the consider-using-ternary branch October 13, 2023 05:44
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.

None yet

3 participants