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

Stabilise rules RUF024 and RUF026 #12026

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 25, 2024

Summary

Stabilise the following rules in the RUF category for Ruff v0.5:

  • mutable-fromkeys-value
  • unnecessary-dict-comprehension
  • default-factory-kwarg
  • parenthesize-chained-operators
  • unsorted-dunder-all
  • unsorted-dunder-slots

These rules have been in the preview category for several minor releases, and were released to users >90 days ago. There are no open issues about any of them, and there have not been any open issues about any of them for several months. I skimmed over the implementations for all of them, and they all look reasonable to me (except for one test fixture for RUF026 that wasn't testing what it was meant to be testing -- I fix that here as part of this PR).

Test Plan

cargo test

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jun 25, 2024
@charliermarsh
Copy link
Member

Generally looks reasonable (though we should review the ecosystem results when we can). ParenthesizeChainedOperators is the only one I'm unsure of because it's more opinionated but let's see what the ecosystem says.

@AlexWaygood AlexWaygood force-pushed the ruff-specific-stabilisations branch from 0506cc5 to f8807c2 Compare June 25, 2024 12:49
@MichaReiser
Copy link
Member

MichaReiser commented Jun 25, 2024

unsorted-dunder-all, unsorted-dunder-slots

could also be considered very-opinionated, but I'm not sure if this should prevent us from stabilizing rules (I don't think we should implement new once until we figured out rule categorization).

Copy link
Contributor

github-actions bot commented Jun 25, 2024

ruff-ecosystem results

Linter (stable)

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

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

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

- airflow/providers/microsoft/azure/hooks/msgraph.py:327:12: PLR1701 [*] Merge `isinstance` calls: `isinstance(data, (BytesIO, bytes, str))`
- airflow/serialization/pydantic/dag.py:45:9: PLR1701 [*] Merge `isinstance` calls
- airflow/serialization/pydantic/taskinstance.py:70:8: PLR1701 [*] Merge `isinstance` calls: `isinstance(x, (BaseOperator, MappedOperator))`

demisto/content (+0 -8 violations, +0 -0 fixes)

- Packs/ANYRUN/Integrations/ANYRUN/ANYRUN.py:196:16: PLR1701 Merge `isinstance` calls: `isinstance(val, dict | list)`
- Packs/AWS-GuardDuty/Integrations/AWSGuardDutyEventCollector/AWSGuardDutyEventCollector.py:33:12: PLR1701 Merge `isinstance` calls: `isinstance(obj, date | datetime)`
- Packs/Base/Scripts/DBotMLFetchData/DBotMLFetchData.py:187:23: PLR1701 Merge `isinstance` calls: `isinstance(x, bool | str)`
- Packs/Base/Scripts/DBotMLFetchData/DBotMLFetchData.py:192:37: PLR1701 Merge `isinstance` calls: `isinstance(v, bool | str)`
- Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py:422:8: PLR1701 Merge `isinstance` calls: `isinstance(obj, list | str)`
- Packs/BluecatAddressManager/Integrations/BluecatAddressManager/BluecatAddressManager.py:256:12: PLR1701 Merge `isinstance` calls
- Packs/ExpanseV2/Scripts/ExpanseEvidenceDynamicSection/ExpanseEvidenceDynamicSection.py:15:8: PLR1701 Merge `isinstance` calls: `isinstance(v, float | int | str)`
- Packs/TOPdesk/Integrations/TOPdesk/TOPdesk.py:577:16: PLR1701 Merge `isinstance` calls: `isinstance(value, bool | str)`

pytest-dev/pytest (+1 -0 violations, +0 -0 fixes)

+ testing/test_runner.py:536:39: RUF024 Do not pass mutable objects as values to `dict.fromkeys`

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
PLR1701 11 0 11 0 0
RUF024 1 1 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -11 violations, +0 -0 fixes in 2 projects; 48 projects unchanged)

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

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

- airflow/providers/microsoft/azure/hooks/msgraph.py:327:12: PLR1701 [*] Merge `isinstance` calls: `isinstance(data, (BytesIO, bytes, str))`
- airflow/serialization/pydantic/dag.py:45:9: PLR1701 [*] Merge `isinstance` calls
- airflow/serialization/pydantic/taskinstance.py:70:8: PLR1701 [*] Merge `isinstance` calls: `isinstance(x, (BaseOperator, MappedOperator))`

demisto/content (+0 -8 violations, +0 -0 fixes)

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

- Packs/ANYRUN/Integrations/ANYRUN/ANYRUN.py:196:16: PLR1701 Merge `isinstance` calls: `isinstance(val, dict | list)`
- Packs/AWS-GuardDuty/Integrations/AWSGuardDutyEventCollector/AWSGuardDutyEventCollector.py:33:12: PLR1701 Merge `isinstance` calls: `isinstance(obj, date | datetime)`
- Packs/Base/Scripts/DBotMLFetchData/DBotMLFetchData.py:187:23: PLR1701 Merge `isinstance` calls: `isinstance(x, bool | str)`
- Packs/Base/Scripts/DBotMLFetchData/DBotMLFetchData.py:192:37: PLR1701 Merge `isinstance` calls: `isinstance(v, bool | str)`
- Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py:422:8: PLR1701 Merge `isinstance` calls: `isinstance(obj, list | str)`
- Packs/BluecatAddressManager/Integrations/BluecatAddressManager/BluecatAddressManager.py:256:12: PLR1701 Merge `isinstance` calls
- Packs/ExpanseV2/Scripts/ExpanseEvidenceDynamicSection/ExpanseEvidenceDynamicSection.py:15:8: PLR1701 Merge `isinstance` calls: `isinstance(v, float | int | str)`
- Packs/TOPdesk/Integrations/TOPdesk/TOPdesk.py:577:16: PLR1701 Merge `isinstance` calls: `isinstance(value, bool | str)`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PLR1701 11 0 11 0 0

@AlexWaygood AlexWaygood changed the title Stabilise rules RUF021-RUF026 Stabilise rules RUF024-RUF026 Jun 25, 2024
@AlexWaygood
Copy link
Member Author

For now, I've struck the following rules (all authored by me 😭) off the list:

  • RUF021 (parenthesize-chained-operators)
  • RUF022 (unsorted-dunder-all)
  • RUF023 (unsorted-dunder-slots)

These rules are all working as designed, but they're all quite opinionated. Right now, it seems like promoting them to stable would be too disruptive for users who select the entire RUF category in their configuration file. Long-term, we'll need to figure out how to stabilise opinionated rules while minimising disruption for our users (which relates to our long-term ambition to rework our rule categorisation)

@AlexWaygood
Copy link
Member Author

For now, I've struck the following rules (all authored by me 😭) off the list:

And I've now struck off RUF025 for similar reasons. Somewhat opinionated; has a fair few ecosystem hits; we can maybe consider it separately, but not for this PR...

@AlexWaygood AlexWaygood changed the title Stabilise rules RUF024-RUF026 Stabilise rules RUF024 and RUF026 Jun 25, 2024
@AlexWaygood AlexWaygood merged commit ae6d966 into ruff-0.5 Jun 25, 2024
16 checks passed
@AlexWaygood AlexWaygood deleted the ruff-specific-stabilisations branch June 25, 2024 15:04
@MichaReiser MichaReiser mentioned this pull request Jun 26, 2024
MichaReiser pushed a commit that referenced this pull request Jun 27, 2024
MichaReiser pushed a commit that referenced this pull request Jun 27, 2024
MichaReiser pushed a commit that referenced this pull request Jun 27, 2024
MichaReiser pushed a commit that referenced this pull request Jun 27, 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.

3 participants