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] Only trigger SIM401 on known dictionaries (SIM401) #15995

Merged
merged 8 commits into from
Feb 7, 2025

Conversation

junhsonjb
Copy link
Contributor

Summary

This change resolves #15814 to ensure that SIM401 is only triggered on known dictionary types. Before, the rule was getting triggered even on types that resemble a dictionary but are not actually a dictionary.

I did this using the is_known_to_be_of_type_dict(...) functionality. The logic for this function was duplicated in a few spots, so I moved the code to a central location, removed redundant definitions, and updated existing calls to use the single definition of the function!

Test Plan

Since this PR only modifies an existing rule, I made changes to the existing test instead of adding new ones. I made sure that SIM401 is triggered on types that are clearly dictionaries and that it's not triggered on a simple custom dictionary-like type (using a modified version of the code in the issue)

The additional changes to de-duplicate is_known_to_be_of_type_dict don't break any existing tests -- I think this should be fine since the logic remains the same (please let me know if you think otherwise, I'm excited to get feedback and work towards a good fix 🙂).

Copy link
Contributor

github-actions bot commented Feb 6, 2025

ruff-ecosystem results

Linter (stable)

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

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

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

- providers/src/airflow/providers/databricks/operators/databricks.py:87:29: SIM401 Use `error = run_output.get("error", run_state.state_message)` instead of an `if` block
- providers/src/airflow/providers/databricks/triggers/databricks.py:107:29: SIM401 Use `error = run_output.get("error", run_state.state_message)` instead of an `if` block

binary-husky/gpt_academic (+0 -1 violations, +0 -0 fixes)

- toolbox.py:437:9: SIM401 Use `current = chatbot._cookies.get("files_to_promote", [])` instead of an `if` block

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

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

- zerver/data_import/mattermost.py:553:9: SIM401 Use `post_team = post.get("team", team_name)` instead of an `if` block
- zproject/computed_settings.py:1186:5: SIM401 Use `path = idp_dict.get("x509cert_path", f"/etc/zulip/saml/idps/{idp_name}.crt")` instead of an `if` block

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM401 5 0 5 0 0

Linter (preview)

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

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

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

- providers/src/airflow/providers/databricks/operators/databricks.py:87:29: SIM401 Use `error = run_output.get("error", run_state.state_message)` instead of an `if` block
- providers/src/airflow/providers/databricks/triggers/databricks.py:107:29: SIM401 Use `error = run_output.get("error", run_state.state_message)` instead of an `if` block

binary-husky/gpt_academic (+0 -1 violations, +0 -0 fixes)

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

- toolbox.py:437:9: SIM401 Use `current = chatbot._cookies.get("files_to_promote", [])` instead of an `if` block

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

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

- zerver/data_import/mattermost.py:553:9: SIM401 Use `post_team = post.get("team", team_name)` instead of an `if` block
- zproject/computed_settings.py:1186:5: SIM401 Use `path = idp_dict.get("x509cert_path", f"/etc/zulip/saml/idps/{idp_name}.crt")` instead of an `if` block

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
SIM401 5 0 5 0 0

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Feb 7, 2025
@MichaReiser
Copy link
Member

Going through the ecosystem results. It's somewhat unfortunate that most of those were true-positives but I still think this is the right trade-off and improvements to the type inference will help to detect the false-negatives in the future.

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks

@MichaReiser MichaReiser enabled auto-merge (squash) February 7, 2025 08:22
@MichaReiser MichaReiser merged commit 349f933 into astral-sh:main Feb 7, 2025
20 checks passed
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.

SIM401 triggers on dict like objects that aren't proper dictionaries
2 participants