Whitespace precommit

[jacomago]

Checklist

• Testing:

  • The feature has automated tests
  • Tests were run
  • If not, explain how you tested your changes

• Documentation:

  • The feature is documented
  • The documentation is up to date
  • Release notes:
    • Added an entry if the change is breaking or significant
    • Added an entry when adding a new feature

[lcaouen]

Hard to review all the files but as it's only removing extra white space, there is no risk to approve it.

[shroffk]

Oh wow...This is a lot

I think that we can configure this check such that we apply it only when we are going to be committing changes to a file. This way we don't have a 1k file changeset and the git history shows everything was last modified by this one commit.

We had followed similar rule with other repos were we applied a new linter... we would only apply it to the files that were part of a new PR

I thought that the above workflow was the default pre-commit behaviour...

[kasemir]

.. apply it only when we are going to be committing changes to a file

Applying this whitespace change to the complete code base risks creating merge conflicts in pending PRs.
Better fix whitespace whenever a module undergoes a functional change.

[shroffk]

can we break this up

1. PR with the pre-commit config
2. PR fixing all the documentation files
3. then we apply the fix with each PR and the files it touches

[jacomago]

Well, I generally disagree with all points:

@lcaouen

Hard to review all the files but as it's only removing extra white space, there is no risk to approve it.

That's why diffing that ignores whitespace exists. Can also run pre-commit run --all-files -v yourself and check that the diff is the same.

@shroffk

Oh wow...This is a lot

It's only whitespace?

I think that we can configure this check such that we apply it only when we are going to be committing changes to a file. This way we don't have a 1k file changeset and the git history shows everything was last modified by this one commit.

That is usually what happens when changing a formatting paradigm, I think it is fine myself.

We had followed similar rule with other repos were we applied a new linter... we would only apply it to the files that were part of a new PR

Oh? Where? I did it in the archiver but not seen that pattern anywhere else. I regret doing it in the archiver and wish I just formatted everything anyway, it caused more problems this way.

I thought that the above workflow was the default pre-commit behaviour...

I don't think so. I think the pre-commit is fix everything as early as possible. (Which I do agree can be annoying).

@kasemir

Applying this whitespace change to the complete code base risks creating merge conflicts in pending PRs. Better fix whitespace whenever a module undergoes a functional change.

Yes, to me that is a good thing. Getting everyone to rebase regularly is IMHO a good practice.

@shroffk

can we break this up

I can have a go.

[shroffk]

Run pre-commit run --show-diff-on-failure --color=always --all-files

can we skip the --all-files part

[shroffk]

I was going to suggest

  - name: Install pre-commit
    run: pip install pre-commit

  - name: Run pre-commit on changed files
    run: |
      pre-commit run \
        --from-ref "${{ github.event.pull_request.base.sha }}" \
        --to-ref "${{ github.event.pull_request.head.sha }}"

to use this

https://pre-commit.com/#pre-commit-run

--from-ref FROM_REF + --to-ref TO_REF: run against the files changed between FROM_REF...TO_REF in git.

[shroffk]

Thank you... I hope this PR wasn't too painful

The to and from is backed by the official pre-commit tool so I think we are on firm grounds here :)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[shroffk]

Also good job on figuring out the how to do it for branch.yml and merge.yml