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

[flake8-simplify] Further simplify to binary in preview for if-else-block-instead-of-if-exp (SIM108) #12796

Merged
merged 13 commits into from
Aug 10, 2024

Conversation

dylwil3
Copy link
Contributor

@dylwil3 dylwil3 commented Aug 10, 2024

In most cases we should suggest a ternary operator, but there are three edge cases where a binary operator is more appropriate.

Given an if-else block of the form

if test:
    target_var = body_value
else:
    target_var = else_value

This PR updates the check for SIM108 to the following:

  • If test == body_value and preview enabled, suggest to replace with target_var = test or else_value
  • If test == not body_value and preview enabled, suggest to replace with target_var = body_value and else_value
  • If not test == body_value and preview enabled, suggest to replace with target_var = body_value and else_value
  • Otherwise, suggest to replace with target_var = body_value if test else else_value

Closes #12189.

@dylwil3
Copy link
Contributor Author

dylwil3 commented Aug 10, 2024

The edge cases in question are pretty rare - not sure if there is a better way to organize the code (for performance and readability) to take advantage of that.

Copy link
Contributor

github-actions bot commented Aug 10, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

apache/airflow (+7 -4 violations, +0 -0 fixes)

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

+ airflow/api/common/mark_tasks.py:311:5: SIM108 Use binary operator `start_date = dag.start_date or execution_date` instead of `if`-`else`-block
- airflow/api/common/mark_tasks.py:311:5: SIM108 Use ternary operator `start_date = dag.start_date if dag.start_date else execution_date` instead of `if`-`else`-block
+ airflow/cli/commands/celery_command.py:213:5: SIM108 Use binary operator `umask = args.umask or conf.get("celery", "worker_umask", fallback=settings.DAEMON_UMASK)` instead of `if`-`else`-block
- airflow/cli/commands/celery_command.py:213:5: SIM108 Use ternary operator `umask = args.umask if args.umask else conf.get("celery", "worker_umask", fallback=settings.DAEMON_UMASK)` instead of `if`-`else`-block
+ airflow/providers/celery/cli/celery_command.py:229:5: SIM108 Use binary operator `umask = args.umask or conf.get("celery", "worker_umask", fallback=settings.DAEMON_UMASK)` instead of `if`-`else`-block
- airflow/providers/celery/cli/celery_command.py:229:5: SIM108 Use ternary operator `umask = args.umask if args.umask else conf.get("celery", "worker_umask", fallback=settings.DAEMON_UMASK)` instead of `if`-`else`-block
+ airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:153:13: SIM108 Use binary operator `namespaces = self.kube_config.multi_namespace_mode_namespace_list or [None]` instead of `if`-`else`-block
+ airflow/providers/databricks/operators/databricks_sql.py:139:17: SIM108 Use binary operator `csv_params = self._csv_params or {}` instead of `if`-`else`-block
- airflow/providers/databricks/operators/databricks_sql.py:139:17: SIM108 Use ternary operator `csv_params = self._csv_params if self._csv_params else {}` instead of `if`-`else`-block
+ airflow/providers/imap/hooks/imap.py:91:13: SIM108 Use binary operator `ssl_context_string = extra_ssl_context or conf.get("imap", "SSL_CONTEXT", fallback=None)` instead of `if`-`else`-block
+ airflow/providers/smtp/hooks/smtp.py:118:13: SIM108 Use binary operator `ssl_context_string = extra_ssl_context or conf.get("smtp_provider", "SSL_CONTEXT", fallback=None)` instead of `if`-`else`-block

scikit-build/scikit-build (+1 -0 violations, +0 -0 fixes)

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

+ skbuild/setuptools_wrap.py:532:5: SIM108 Use binary operator `cmake_install_target = cmake_install_target_from_command or cmake_install_target_from_setup` instead of `if`-`else`-block

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

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

+ zerver/webhooks/bitbucket3/view.py:78:5: SIM108 Use binary operator `topic_name = include_title or "Bitbucket Server Ping"` instead of `if`-`else`-block
- zerver/webhooks/bitbucket3/view.py:78:5: SIM108 Use ternary operator `topic_name = include_title if include_title else "Bitbucket Server Ping"` instead of `if`-`else`-block

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM108 14 9 5 0 0

@charliermarsh
Copy link
Member

Sweet, thanks!

@charliermarsh charliermarsh self-assigned this Aug 10, 2024
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Aug 10, 2024
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 good to me. I just added a check to ensure there's no effect (like a function call) in the test, since it no longer gets repeated.

@charliermarsh charliermarsh enabled auto-merge (squash) August 10, 2024 16:41
@charliermarsh charliermarsh merged commit 0c2b88f into astral-sh:main Aug 10, 2024
19 checks passed
@dylwil3 dylwil3 deleted the sim108-boolean-simplify branch August 10, 2024 18:06
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.

SIM108 suggested for code block that could have been simplified to an or
2 participants