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

[refurb] - add bool-literal-compare with fix (FURB149) #9539

Closed
wants to merge 5 commits into from

Conversation

diceroll123
Copy link
Contributor

@diceroll123 diceroll123 commented Jan 15, 2024

Summary

Add bool-literal-compare (FURB149) with fix

See: #1348

Test Plan

cargo test

@diceroll123 diceroll123 force-pushed the add-FURB149 branch 2 times, most recently from cebe846 to cf1d0a4 Compare January 15, 2024 23:07
Copy link
Contributor

github-actions bot commented Jan 15, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+1673 -0 violations, +0 -0 fixes in 5 projects; 38 projects unchanged)

apache/airflow (+616 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/cli/commands/celery_command.py:90:8: FURB149 Comparison to `False` should be `not cond`
+ airflow/cli/commands/connection_command.py:371:8: FURB149 Comparison to `True` should be `cond`
+ airflow/cli/commands/dag_command.py:139:8: FURB149 Comparison to `False` should be `not cond`
+ airflow/cli/commands/scheduler_command.py:83:12: FURB149 Comparison to `False` should be `not cond`
+ airflow/cli/commands/triggerer_command.py:39:8: FURB149 Comparison to `False` should be `not cond`
+ airflow/configuration.py:318:16: FURB149 Comparison to `True` should be `cond`
+ airflow/jobs/triggerer_job_runner.py:192:8: FURB149 Comparison to `False` should be `not cond`
+ airflow/jobs/triggerer_job_runner.py:281:14: FURB149 Comparison to `False` should be `not cond`
+ airflow/models/abstractoperator.py:193:12: FURB149 Comparison to `True` should be `cond`
+ airflow/models/abstractoperator.py:193:30: FURB149 Comparison to `False` should be `not cond`
+ airflow/models/dagrun.py:1473:17: FURB149 Comparison to `False` should be `not cond`
+ airflow/models/taskinstance.py:335:8: FURB149 Comparison to `True` should be `cond`
+ airflow/models/taskinstance.py:990:8: FURB149 Comparison to `True` should be `cond`
+ airflow/operators/python.py:282:16: FURB149 Comparison to `True` should be `cond`
+ airflow/providers/amazon/aws/operators/emr.py:1481:20: FURB149 Comparison to `True` should be `cond`
+ airflow/providers/apache/kafka/operators/consume.py:103:12: FURB149 Comparison to `True` should be `cond`
+ airflow/providers/atlassian/jira/sensors/jira.py:125:12: FURB149 Comparison to `True` should be `cond`
+ airflow/providers/celery/cli/celery_command.py:106:8: FURB149 Comparison to `False` should be `not cond`
... 598 additional changes omitted for project

bokeh/bokeh (+367 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ src/bokeh/command/bootstrap.py:116:8: FURB149 Comparison to `False` should be `not cond`
+ src/bokeh/command/bootstrap.py:118:10: FURB149 Comparison to `False` should be `not cond`
+ src/bokeh/server/tornado.py:623:16: FURB149 Comparison to `True` should be `cond`
+ src/bokeh/server/tornado.py:625:40: FURB149 Comparison to `False` should be `not cond`
+ tests/integration/tools/test_custom_action.py:46:16: FURB149 Comparison to `True` should be `cond`
+ tests/unit/bokeh/application/handlers/test_code_runner.py:108:16: FURB149 Comparison to `False` should be `not cond`
+ tests/unit/bokeh/application/handlers/test_code_runner.py:116:16: FURB149 Comparison to `True` should be `cond`
+ tests/unit/bokeh/application/handlers/test_code_runner.py:179:16: FURB149 Comparison to `False` should be `not cond`
+ tests/unit/bokeh/application/handlers/test_code_runner.py:45:16: FURB149 Comparison to `False` should be `not cond`
+ tests/unit/bokeh/application/handlers/test_code_runner.py:49:16: FURB149 Comparison to `False` should be `not cond`
... 357 additional changes omitted for project

rotki/rotki (+633 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ package.py:863:12: FURB149 Comparison to `False` should be `not cond`
+ packaging/docker/entrypoint.py:124:8: FURB149 Comparison to `True` should be `cond`
+ rotkehlchen/accounting/debugimporter/json.py:107:16: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/debugimporter/json.py:107:56: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/debugimporter/json.py:45:12: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/export/csv.py:133:12: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/export/csv.py:182:46: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/export/csv.py:184:43: FURB149 Comparison to `True` should be `cond`
+ rotkehlchen/accounting/export/csv.py:200:12: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/export/csv.py:316:12: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/pot.py:165:20: FURB149 Comparison to `True` should be `cond`
+ rotkehlchen/accounting/pot.py:182:12: FURB149 Comparison to `True` should be `cond`
+ rotkehlchen/accounting/pot.py:386:16: FURB149 Comparison to `True` should be `cond`
+ rotkehlchen/accounting/rules.py:60:12: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/accounting/structures/processed_event.py:207:36: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/api/rest.py:1288:12: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/api/rest.py:1302:12: FURB149 Comparison to `False` should be `not cond`
+ rotkehlchen/api/rest.py:1622:8: FURB149 Comparison to `False` should be `not cond`
... 615 additional changes omitted for project

zulip/zulip (+31 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ corporate/lib/stripe.py:1675:20: FURB149 Comparison to `True` should be `cond`
+ corporate/lib/stripe.py:1779:24: FURB149 Comparison to `True` should be `cond`
+ corporate/lib/stripe.py:3562:16: FURB149 Comparison to `False` should be `not cond`
+ corporate/lib/stripe.py:3594:20: FURB149 Comparison to `False` should be `not cond`
+ corporate/lib/stripe.py:3956:16: FURB149 Comparison to `False` should be `not cond`
+ corporate/lib/stripe.py:3986:20: FURB149 Comparison to `False` should be `not cond`
+ corporate/lib/stripe.py:4389:16: FURB149 Comparison to `False` should be `not cond`
+ corporate/lib/stripe.py:4419:20: FURB149 Comparison to `False` should be `not cond`
+ zerver/actions/create_realm.py:216:40: FURB149 Comparison to `True` should be `cond`
+ zerver/actions/scheduled_messages.py:144:12: FURB149 Comparison to `True` should be `cond`
... 21 additional changes omitted for project

indico/indico (+26 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ indico/cli/devserver.py:208:14: FURB149 Comparison to `True` should be `cond`
+ indico/cli/devserver.py:70:16: FURB149 Comparison to `True` should be `cond`
+ indico/core/db/sqlalchemy/links.py:217:30: FURB149 Comparison to `False` should be `not cond`
+ indico/core/db/sqlalchemy/links.py:244:30: FURB149 Comparison to `False` should be `not cond`
+ indico/core/db/sqlalchemy/links.py:258:30: FURB149 Comparison to `False` should be `not cond`
+ indico/core/db/sqlalchemy/links.py:272:30: FURB149 Comparison to `False` should be `not cond`
+ indico/core/db/sqlalchemy/links.py:286:30: FURB149 Comparison to `False` should be `not cond`
+ indico/core/db/sqlalchemy/links.py:300:30: FURB149 Comparison to `False` should be `not cond`
+ indico/modules/events/editing/service.py:259:14: FURB149 Comparison to `False` should be `not cond`
+ indico/modules/events/fields_test.py:57:12: FURB149 Comparison to `False` should be `not cond`
... 16 additional changes omitted for project

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB149 1673 1673 0 0 0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@diceroll123 diceroll123 marked this pull request as draft January 16, 2024 03:07
@diceroll123 diceroll123 marked this pull request as ready for review January 16, 2024 03:25
@tjkuson
Copy link
Contributor

tjkuson commented Jan 21, 2024

What overlap does this have with true-false-comparison (E712)?

@diceroll123
Copy link
Contributor Author

Hrm, in theory, FURB149 could fix it before E712 fixes it, or, E712 fixes it and then FURB149 fixes it afterward.

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.

This looks pretty similar to true-false-comparison, do you mind comparing the two rules?

@charliermarsh
Copy link
Member

Ugh, sorry, I missed @tjkuson comment 🤦

@charliermarsh
Copy link
Member

But do you know if the rules catch the same cases, @diceroll123?

@diceroll123
Copy link
Contributor Author

diceroll123 commented Jan 24, 2024

Made an update; I fixed an edge case or two, and narrowed it to disallow chained comparisons while doing the testing below, and after some consideration I've also made the fixes both set to unsafe.

Semantically speaking, I suppose FURB149 is more unsafe than E712, but they definitely don't do the same thing. The major difference is dropping the explicit equivalence operator in favor of the hand-wavy truthiness/falsiness. Falseyness? idk

Here's a comparison of both E712 and FURB149 autofixes ran on the same file:

Running E712 on the E712.py text fixture (with fixes) gives this:
image


Running FURB149 on the same file with fixes:

image

@MichaReiser
Copy link
Member

Thanks @diceroll123 for working on this PR and writing up the difference to true-false-comparision.

I understand that this rull and bool-literal-compare identify the same code smell but provide different fixes. We should only have one rule, or the experience for users is confusing (what fix am I supposed to use and why?) I see two solutions, but this requires more design before opening a follow-up PR:

  • An option is to extend bool-literal-compare to specify the preferred fix.
  • Wait until Ruff has access to types and use the type information to decide if the safe fix is to fix to x is True or x.

I'm happy to check in with the team to see what they think of adding an option to true-false-comparision to direct the fix but I'll close this PR because Ruff should only have a single rule per code-smell.

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Mar 28, 2024
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.

4 participants