fprettify icon indicating copy to clipboard operation
fprettify copied to clipboard

Formatting with black and isort

Open max-models opened this issue 4 months ago • 10 comments

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.

max-models avatar Nov 11 '25 22:11 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).

nbehrnd avatar Nov 12 '25 08:11 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).

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.

max-models avatar Nov 12 '25 10:11 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.

nbehrnd avatar Nov 12 '25 10:11 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.

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.

max-models avatar Nov 12 '25 13:11 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.

nbehrnd avatar Nov 21 '25 18:11 nbehrnd

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.

pseewald avatar Nov 21 '25 18:11 pseewald

@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 avatar Nov 21 '25 22:11 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.

max-models avatar Nov 21 '25 22:11 max-models

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.

pseewald avatar Nov 22 '25 10:11 pseewald

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

max-models avatar Nov 22 '25 11:11 max-models