Formatting with black and isort

[max-models]

Since this project is all about code formatting, I thought it would be a nice idea to also format the python code, so I ran two of the most popular python code formatters (black and isort) on the Python files in the repo.

I also added a check in the Github workflow to verify that the code is formatted with both black and isort.

[nbehrnd]

@max-models

I also added a check in the Github workflow to verify that the code is formatted with both black and isort.

I propose to edit the PR to equally amend the already present pre-commit hook file. Once locally activated, this already can assist (at local level) en route to eventually file a PR. Be welcome to pick, edit, adjust to your preference from example below:

repos: - repo: https://github.com/pre-commit/pre-commit-hooks rev: v5.0.0 hooks: - id: trailing-whitespace exclude: ^tests/resources/ - repo: https://github.com/psf/black rev: 25.1.0 hooks: - id: black - repo: https://github.com/PyCQA/flake8 rev: 7.1.1 hooks: - id: flake8 name: "flake8 (advisory only, it does not block a commit)" args: - --exclude=.*/site-packages/.* - --filename=.*\.py - --max-line-length=90 - --show-source - --statistics - --count - --exit-zero verbose: true always_run: true - repo: https://github.com/pre-commit/mirrors-isort rev: v5.10.1 hooks: - id: isort name: sorts imports args: ["--profile", "black", "--filter-files", "--line-width=99"]

The file defines them explicitly, in addition to templates git init provides in .git/hooks/. If set well, they equally work fine with commitizen, too (because multiple commits by @gnikit adhere to the pattern of conventional commits -- but like pre-commit hooks, it is an opt-in).

[max-models]

@max-models

I also added a check in the Github workflow to verify that the code is formatted with both black and isort.

I propose to edit the PR to equally amend the already present pre-commit hook file. Once locally activated, this already can assist (at local level) en route to eventually file a PR. Be welcome to pick, edit, adjust to your preference from example below:

repos: - repo: https://github.com/pre-commit/pre-commit-hooks rev: v5.0.0 hooks: - id: trailing-whitespace exclude: ^tests/resources/ - repo: https://github.com/psf/black rev: 25.1.0 hooks: - id: black - repo: https://github.com/PyCQA/flake8 rev: 7.1.1 hooks: - id: flake8 name: "flake8 (advisory only, it does not block a commit)" args: - --exclude=.*/site-packages/.* - --filename=.*\.py - --max-line-length=90 - --show-source - --statistics - --count - --exit-zero verbose: true always_run: true - repo: https://github.com/pre-commit/mirrors-isort rev: v5.10.1 hooks: - id: isort name: sorts imports args: ["--profile", "black", "--filter-files", "--line-width=99"]

The file defines them explicitly, in addition to templates git init provides in .git/hooks/. If set well, they equally work fine with commitizen, too (because multiple commits by @gnikit adhere to the pattern of conventional commits -- but like pre-commit hooks, it is an opt-in).

Ok, good idea! I can do that, although I personally don't like to add too much to the pre-commit hooks, since it often becomes a bit too annoying.

[nbehrnd]

@max-models I agree with you. Each hook and test is like a droplet of glue -- if there are too many, they i) can hinder each other's action. Instead of providing a tangible benefit to the project per sé, ii) they then slow down progress.

[max-models]

@max-models I agree with you. Each hook and test is like a droplet of glue -- if there are too many, they i) can hinder each other's action. Instead of providing a tangible benefit to the project per sé, ii) they then slow down progress.

Agreed :) Sometimes I feel like there is so much glue that I can't even make a dirty temporary commit, so I just turn off all the pre-commit hooks instead. In any case, using the hooks is optional, and the formatting is checked with Github actions.

I added a .pre-commit-config.yaml with black and isort hooks in 3815c71.

[nbehrnd]

@max-models Today I learned ruff actually can provide both a normal check / lint / reformat (like black) and optionally provide isort like rearrangements (ruff check --select I --fix). See for instance Corey Schafer's video. My «accident» was to type a --fix (to ruff) just after reading the same flag to fortitude-lint in the history of another tab of the terminal. On the other hand, black is more strict / comes with less adjustable parameters, which can be good, too.

[pseewald]

Since this project is all about code formatting, I thought it would be a nice idea to also format the python code

Agreed, thanks for doing this pr. There will be many conflicts with existing prs but this shouldn't be a blocker because conflicts can just be resolved using "ours" and then redo the formatting after the merge, I guess.

[max-models]

@max-models Today I learned ruff actually can provide both a normal check / lint / reformat (like black) and optionally provide isort like rearrangements (ruff check --select I --fix). See for instance Corey Schafer's video. My «accident» was to type a --fix (to ruff) just after reading the same flag to fortitude-lint in the history of another tab of the terminal. On the other hand, black is more strict / comes with less adjustable parameters, which can be good, too.

I'm using ruff in a separate project (struphy) and it's nice because it is very fast, but I like the fact that black has less configuration, also it's a more mature project so I would probably start off by using it. The ruff check is also really nice for linting (and even fixing some errors), I usually use it manually to clean up code.

[max-models]

Since this project is all about code formatting, I thought it would be a nice idea to also format the python code

Agreed, thanks for doing this pr. There will be many conflicts with existing prs but this shouldn't be a blocker because conflicts can just be resolved using "ours" and then redo the formatting after the merge, I guess.

Agreed! Almost all conflicts should be fixable with this method, or simply by running black and isort on the branches in the PRs before merging. Alternatively, you can merge a few of the PRs first before we add the formatting check.

[gnikit]

I think this CI workflow can largely be discarded in favour of pre-commit CI. I would only have to enable the webhook in the Settings and make it a mandatory check for the branch ruleset.
(Some others mentioned pre-commit in the discussion). Generally, it's a good idea to define your linting, formating, etc. in one place and then have pre-commit hooks locally and on the CI use them.
Less maintenance work, overall.

[max-models]

I hear you, it's a preference I guess. I prefer to avoid pre-commit hooks because they sometimes just gets in the way. Also, I tend to not always have them activated, and I see the same tendency among many others. As soon as they cause a moment of annoyance, people turn them off and then the formatting goes out the window. I feel like it's simpler to just let the developers choose themselves at which stage they would like to do the formatting. The pre-commit config file is there for anyone to activate if they would like, isn't that enough?

[gnikit]

people can do as they wish on their machines, having a pre-commit yaml config does not change that.
The yaml config is what is read from the pre-commit.ci which will then automatically run the checks on GitHub.

[max-models]

Ah, ok sorry I see what you mean now, I didn't know this was possible. I can update the CI.

[gnikit]

yeah so basically how this is done is:

• repo configures isort, black, etc. in pyproject/setup.cgf or explicit config files for each tool
• repo adds simple pre-commit-config.yaml see
• repo/org admin enable the pre-commit.ci webhook
• Optional: repo admin updates branch protection rules to include pre-commit webhook

Now every commit that is pushed on GitHub will be passed through isort/black/etc. and it will fail if any of the linters/formatter fail. You only have a single source of truth to worry about (the pre-commit config).

That being said for large projects, even locally, I find that it helps developing in an organised clean manner (abiding by linters, formatters, making atomic commits etc.), so I will almost certainly use pre-commit and run tests, when I commit and definitely before I push.

[max-models]

@pseewald what is your opinion, would you like to use a pre-commit.ci webhook? As of now, the static analysis job just runs pre-commit run --all-files

[pseewald]

Alternatively, you can merge a few of the PRs first before we add the formatting check.

The only pr I wanted to get in first (#188) is merged. I will selectively merge other pr's as I find time but the order doesn't matter, so please go ahead with this pr.

[max-models]

I updated this branch by formatting master and merging it. I also verified that the only diffs of the Python files in this PR come from black and isort as follows:

> git checkout master > git pull > git checkout -b master-formatted > black . > isort . > git add -u > git commit -m "Formatting with black and isort" > git checkout formatting-with-black-and-isort > git diff master-formatted --name-only .github/workflows/static_analysis.yml .pre-commit-config.yaml