Skip to content

Run ruff ci but ignore errors #296

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

Merged

Conversation

steinmn
Copy link
Contributor

@steinmn steinmn commented Aug 21, 2025

Adds ruff validation to make sure we don't regress on formatting and showing our progress towards #258

Thanks to --exit-zero the tests still passes despite us still having 296 errors.

@steinmn
Copy link
Contributor Author

steinmn commented Aug 21, 2025

https://github.com/steinmn/zaptec/actions/runs/17136448312/job/48613481883 demonstrates the ruff check failing due to formatting errors

@sveinse sveinse added the code quality Issues related to the quality of the code label Aug 22, 2025
@sveinse
Copy link
Collaborator

sveinse commented Aug 22, 2025

LGTM.

It's a little bit annoying that GH only presents 10 warnings & errors in GHA runs. But that's out of our hands, so I think this is ok. It does require some extra steps to check the result, so for that it might be a little hidden.

Is there a way to enforce that there should not be any lint errors in the touched line of the PR?

@steinmn
Copy link
Contributor Author

steinmn commented Aug 22, 2025

Is there a way to enforce that there should not be any lint errors in the touched line of the PR?

Not sure if there is any simple way to do that. You could of course parse the results somehow, but that seems like a lot of effort that I'd rather put towards just solving as many errors as possible and get to the point where we can remove the exit-zero workaround. (I have some follow-up-branches that remove quite a few errors)

@sveinse
Copy link
Collaborator

sveinse commented Aug 22, 2025

Google AI propose:

- name: Run Ruff on changed files
  run: |
    CHANGED_FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD -- '*.py' | tr '\n' ' ')
    if [ -n "$CHANGED_FILES" ]; then
      ruff check $CHANGED_FILES
    else
      echo "No Python files changed in this PR, skipping Ruff check."
    fi

Note: While this approach can reduce the scope of the check, it's important to remember that linting errors can sometimes be non-local. A change in one part of a file might introduce an error in an unchanged section, which this method might not catch. For comprehensive linting, running Ruff on the entire codebase remains the most robust approach.

Some other finds:

If it turns out to be very troublesome to enforce successful lint on PRs, I think it should be a priority to fix all the current issues. We want to turn on the lint failure as soon as possible.

@steinmn
Copy link
Contributor Author

steinmn commented Aug 22, 2025

The AI proposal only really works as long as we don't touch any of the files that still have ruff errors without fixing all the ruff errors in the touched file. I'd rather go with just prioritizing fixing ruff errors. As mentioned, I have been making some progress there, but it's easiest to see the effect if we merge this and I can rebase my branches onto the new master and make PRss from that.

@sveinse
Copy link
Collaborator

sveinse commented Aug 22, 2025

I forgot the conclusion of my post, and I agree: It's not that straight forward to implement, so I'm also keen on just fixing the lint errors than making a lot of machinery to circumvent it.

I'll merge this PR.

@sveinse sveinse merged commit 756e98f into custom-components:master Aug 22, 2025
4 checks passed
@steinmn steinmn deleted the run-ruff-ci-but-ignore-errors branch August 22, 2025 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Issues related to the quality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants