Style checker on PRs does not actually check anything

[agriyakhetarpal]

PyBaMM Version

develop

Python Version

N/A

Describe the bug

The style job in the test_on_push.yml workflow does not notice any files to lint/auto-format and therefore is passing silently.

Steps to Reproduce

It can be noticed in the checks on any of the PRs, for example:

https://github.com/pybamm-team/PyBaMM/actions/runs/7009110613/job/19066860618?pr=3569#step:4:65

Relevant log output

No response

[kratman]

I think the command

pre-commit run ruff

skips.

The easiest fix would be to just do

pre-commit run ruff --all

Though that only runs ruff, so maybe

pre-commit run --all

is better.

[kratman]

There are ways to get pre-commit to run on only changed files as well, but using --all is the easiest route

[agriyakhetarpal]

I would suggest running it on just the changed files in a PR. The style check is needed to pass for other checks to start, so it would be a nuisance if it fails due to issues in unchanged files.

[shubhambhar007]

@kratman @agriyakhetarpal I was exploring on this and came across this command for only the changed files in the PR.

pre-commit run --files $(git diff --name-only HEAD^)

If it looks fine , can i please work on this issue ?
Thanks :D

[kratman]

@shubhambhar007 Yep feel free to work on the issue.

[agriyakhetarpal]

Fixed in #3580

[agriyakhetarpal]

I think this isn't working, again: here are some recent logs where pre-commit reports that there are no files to check.

I think we can run it on all files as well – if anything fails, it will be a one-time change, on further PRs, we will then have a clean output from this command.

This is what I use for https://pybamm.org –

@nox.session(name="lint", venv_backend="virtualenv")
def lint(session):
    """Install 'pre-commit' and run linting on all files."""
    session.install("pre-commit")
    session.run("pre-commit", "run", "--all-files", "--show-diff-on-failure")

which is essentially very similar to what the Scientific Python guidelines recommend (except for the fact that they recommend PyLint too; we usually skip that)

[kratman]

I think we don't need the extra nox function. We just add the --all in the workflow. Nice and easy

[shubhambhar007]

@agriyakhetarpal any idea as to why this might not be working?
We were initially trying with --all but then moved to this to make it run only for the diffs , it was working on my local and I also saw a few other PRs.

While --all may be slow but i think it is best to have it running slowly rather than nor running at all at times.

[kratman]

@shubhambhar007 the add probably only looks for locally changed files that were not committed while PRs have all changes committed. I think when I have seen this done on other projects they used a diff to a commit hash.

Running with --all is still orders of magnitude faster than the rest of our checks. All changes to style settings are supposed to have isolated commits to hide them from git blame, so I don't think there are really any downsides to using --all

[agriyakhetarpal]

I think when I have seen this done on other projects they used a diff to a commit hash.

I guess it would be great to migrate to the pre-commit GitHub Action for this in the long term to get results that bear similarity with the pre-commit bot

[shubhambhar007]

@agriyakhetarpal got it, thanks :D
Yes , i agree, safe side is using --all .