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

Enable Ruff Preview #115687

Draft
wants to merge 6 commits into
base: dev
Choose a base branch
from
Draft

Enable Ruff Preview #115687

wants to merge 6 commits into from

Conversation

autinerd
Copy link
Contributor

Proposed change

This enables the Ruff preview. The option explicit-preview-rules = true is used so that no new preview rules are enabled unless explicitly defined.

This only enables more findings and fixes for stable rules.

The option output-format = "concise" is there because in preview mode, Ruff defaults to the full output.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Overall I don't think we should enable the ruff preview rules. It's fine to apply them locally from time to time and suggest a PR with the changes but I'm against using them by default.

They are considered preview for a reason after all. Btw. that's similar to how I handled new pylint rules in the past.

homeassistant/components/airly/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/bluetooth/match.py Outdated Show resolved Hide resolved
homeassistant/components/bluetooth/match.py Outdated Show resolved Hide resolved
homeassistant/components/cloud/alexa_config.py Outdated Show resolved Hide resolved
homeassistant/components/cloud/google_config.py Outdated Show resolved Hide resolved
homeassistant/components/eafm/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/egardia/alarm_control_panel.py Outdated Show resolved Hide resolved
homeassistant/components/emulated_hue/config.py Outdated Show resolved Hide resolved
tests/auth/mfa_modules/test_insecure_example.py Outdated Show resolved Hide resolved
call, msg = await assert_request_calls_service(
_, msg = await assert_request_calls_service(
Copy link
Member

Choose a reason for hiding this comment

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

call might technically be unused. However, I would recommend against replacing it with _. There isn't really a performance benefit in doing so and it can help with debugging.

Personally, I suggest we disable that rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, pylint is the same opinion as Ruff is and puts it as "Unused variable"

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft April 16, 2024 10:24
@autinerd
Copy link
Contributor Author

Just for clarification, these stable rules are extended in preview mode (which have occurrences in the codebase):

  • F841: Local variable is assigned to but never used
    This rule triggers on unpacked variables as well.

  • SIM103: Return the condition directly
    It triggers on implicit else cases as well, such as:

    if a:
        return True
    return False

    In stable mode, it only triggers when the return False is in an explicit else.

  • SIM300: Yoda conditions are discouraged
    It triggers on constant lists, sets, dicts as well, in stable mode it only triggers on numbers and strings

  • E721: Use is and is not for type comparisons, or isinstance() for isinstance checks
    It allows is and is not as well, not only isinstance

  • C419: Unnecessary list comprehension
    This adds sum,min and max to the functions for which the unnecessary comprehension is flagged.

@cdce8p
Copy link
Member

cdce8p commented Apr 16, 2024

Should we then ignore the rule completely? I think the introduction in #113015 was not seen as a problem because the explicit else was rarely used.

I don't have a problem with disabling the rule, because you are right, building big boolean constructions can lead to quite ugly code.

IMO it's a bit unfortunate that they extend it that way and not just create a new rule, but given that I would suggest we disable it.

Same story for F841. It works well for normal assignment, but falls short with the extension to tuple assignments IMO.

The other changes seem fine but as I explained earlier, I'd suggest we only commit these without enabling the preview mode in general.

@autinerd
Copy link
Contributor Author

I can understand why they extended it that way, but the explicit else just didn't have had a big impact because of the rules which transform explicit else into implicit else. I will add it to the ignore list in a separate PR.

With F841 however, I would say to keep this, because this kind of discarding of tuple unpacking variables is done the same in many languages like Rust, C# etc., and is also quite common in this codebase already.

@autinerd autinerd mentioned this pull request Apr 17, 2024
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants