Precommit

[ntchen]

📝 Description

Type: 🎢 infrastructure

.pre-commit-config.yaml is added. It integrates isort (new), ruff and black. ruff and black will check the local rules, e.g., .ruff.toml. I did not do pre-commit run --all-files, since it influences too many files. For new commits, it only scans the modified files.

Text need to be added to the documents:

Setting Up Pre-Commit Hooks

To ensure code quality and consistency, we use pre-commit hooks in this project. Please follow these steps to set up pre-commit on your local machine:

Install pre-commit by running:
pip install pre-commit

Set up the pre-commit hooks with:
pre-commit install

This needs to be done only once per repository. The pre-commit hooks will now automatically run on each commit to ensure your changes meet our code quality standards.

Also, link issues affected by this pull request by using the keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.
N/A

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

• Testing pipeline
• Other method (describe)
• My changes can't be tested (explain why)
• it is an independent file

☑️ Checklist

• I requested two reviewers for this pull request
• I updated the documentation according to my changes
• I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

[ntchen]

if necessary, we can replace ruff (linter and formatter) with flak8 (linter only), since there is black (formatter).

  - repo: https://github.com/pycqa/flake8
    rev: 7.0.0
    hooks:
      - id: flake8

[ntchen]

I just noticed another pull request for pre-commit configuration after creating this, so this is not really necessary, but there is still some difference. #2487

[Rodot-]

I just noticed another pull request for pre-commit configuration after creating this, so this is not really necessary, but there is still some difference. #2508

PR #2487

[Rodot-]

@AlexHls @andrewfullard How does this compare to the other pre-commit PR?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.74%. Comparing base (17c5da9) to head (d624d56).
Report is 31 commits behind head on master.

❗ Current head d624d56 differs from pull request most recent head f3c2a04. Consider uploading reports for the commit f3c2a04 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2509      +/-   ##
==========================================
+ Coverage   68.63%   68.74%   +0.10%     
==========================================
  Files         159      165       +6     
  Lines       13749    13999     +250     
==========================================
+ Hits         9437     9623     +186     
- Misses       4312     4376      +64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AlexHls]

This adds different hooks and could be merged independently (I think @andrewfullard was against action actionlint anyways, so that PR can potentially be closed anyway)

[AlexHls]

I would not run black and ruff at the same time. The ruff config mimicks blak formatting, however there are some subtle differences.

Ultimately both formating and linting should be taken care of by ruff. Having both black and ruff doing the same thing will only lead to conflicts.

Since this PR should only affect new files the choice of formatter should not matter and as such I would vote for ruff being the sole formatting tool. Besides, the pyproject.toml already contains the formatting configuration for ruff.

[andrewfullard]

Does ruff perform the same formatting as black? Do they follow the same rules?

[AlexHls]

Not necessarily. In principle it should be a black drop in replacement (see docs). However, in a very limited number of cases it will fail the black checks. In an ideal word there would be a 'all in one' switch where the formatting provider is changed to ruff.

For reference, my IDE automatically applies ruff formatting and I never had issues. But I recall other people having issues.

In my opinion, ruff should be adopted as the default formatter moving forward and black should only be used case-by-case where the pipeline rejects the formatting.

[ntchen]

removed black

[AlexHls]

Why does this line change? Should be cleaned up.

[ntchen]

there was a space in the end and fixed by the pre-commit. 200 files have similar problems, if use pre-commit run --all-files, but not necessary to scan all now.

[andrewfullard]

My main concern with all precommit hooks is any potential barrier to development. Will this require a change to the TARDIS environment to use these tools, for example? How seamless is it?

[ntchen]

My main concern with all precommit hooks is any potential barrier to development. Will this require a change to the TARDIS environment to use these tools, for example? How seamless is it?

It is seamless and does not require a change. It checks the local rules, e.g., .ruff.toml, as written in the description. That's why I followed the previous setup to use ruff and black; otherwise, it can be ruff only or black + flake8 (this also answers the above @AlexHls ).

Users have the option to not use it. When utilized, it only scans the committed files.

[andrewfullard]

My main concern with all precommit hooks is any potential barrier to development. Will this require a change to the TARDIS environment to use these tools, for example? How seamless is it?

It is seamless and does not require a change. It checks the local rules, e.g., .ruff.toml, as written in the description. That's why I followed the previous setup to use ruff and black; otherwise, it can be ruff only or black + flake8 (this also answers the above @AlexHls ).

Users have the option to not use it. When utilized, it only scans the committed files.

Excellent. Please add the instructions for setup (the ones you have in the PR description are fine) to the developer's guide part of the documentation.

[andrewfullard]

@ntchen could you add the instructions for setup? I am also concerned about the black+ruff combo as Alex mentioned

[ntchen]

@ntchen could you add the instructions for setup? I am also concerned about the black+ruff combo as Alex mentioned

black has been removed and the instructions are in the document. can you take a look? @andrewfullard

[AlexHls]

@andrewfullard what do you think of adding a formatting commit (either in this pr or a separate one), enforcing ruff formatting and adding a ruff pre-commit hook to ensure consistent formatting. If we're concerned about the git blame history, a .git-blame-ignore-revs entry could be an option.
I'm just concerned that otherwise is may take quite a while until ruff formatting takes over 'naturally'. (Not sure if this would require changes to the cicd.)

[andrewfullard]

@andrewfullard what do you think of adding a formatting commit (either in this pr or a separate one), enforcing ruff formatting and adding a ruff pre-commit hook to ensure consistent formatting. If we're concerned about the git blame history, a .git-blame-ignore-revs entry could be an option. I'm just concerned that otherwise is may take quite a while until ruff formatting takes over 'naturally'. (Not sure if this would require changes to the cicd.)

Well, given that ruff and black are currently interchangeable aside from line length, which we can set in ruff.toml, I don't think it's an issue unless you want to apply all of ruff's rules such as import sorting from the start.