Build is green

Patch application report for D7502 (id=27213)

Rebasing onto 4083f79618...

Current branch diff-target is up to date.

Changes applied before test

commit 8d2b5beed272291e852d319b6e9a7832ead694af
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Apr 5 11:55:32 2022 +0200

    python: Reformat code with black 22.3.0
    
    Related to T3922

commit fcddc842d9e1ca586b4bdc616037bc35c6155859
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Apr 5 11:51:41 2022 +0200

    pre-commit, tox: Bump black from 19.10b0 to 22.3.0
    
    black is considered stable since release 22.1.0 and the version
    we are currently using is quite outdated and not compatible with
    click 8.1.0, so it is time to bump it to its latest stable release.
    
    Related to T3922

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/86/ for more details.

(sorry, typing... please hold)

Thanks for looking into this!

Instead of running black swh, I would suggest using pre-commit run -a black to make sure the proper version is used, and all affected files are properly reformatted (we have some python files not in the swh directory).

I've tried this out using mr (I'm not sure why the mass-change script is needed, mr run works just fine, and gives us plenty of chances to inspect the changes instead of yolo-pushing everything!):

git clone https://forge.softwareheritage.org/source/swh-environment.git mass-swh-changes
cd mass-swh-changes
readlink -f .mrconfig >> ~/.mrtrust
mr -j4 up

# (removed the snippets section from .mrconfig)

mr run sed -i s/19.10b0/22.3.0/ .pre-commit-config.yaml tox.ini
mr run git add .pre-commmit-config.yaml tox.ini 
mr run git commit -n -m "pre-commit, tox: Bump black from 19.10b0 to 22.3.0

black is considered stable since release 22.1.0 and the version
we are currently using is quite outdated and not compatible with
click 8.1.0, so it is time to bump it to its latest stable release."

mr run pre-commit run -a black
mr run git commit -m 'Reformat code with black 22.3.0'

Looks like the following repositories will need post-blackening finagling to make the reformatted docstrings fit the hard 88-char limit.

• swh-deposit
• swh-icinga-plugins
• swh-loader-cvs
• swh-loader-mercurial
• swh-loader-svn
• swh-scheduler
• swh-search
• swh-web

swh-scanner is failing mypy for an unrelated reason (importlib_metadata missing stubs)

I thought we could introduce flake8-bugbear for line length limits, as suggested by black's docs: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length, but I can't seem to find a way to *only* enable B950 and not the other bugbear warnings. sigh.

So, I guess we get to manually fix up when black decides that a few characters over is fine. Or we can bump flake8's line length limit to 90.

Finally, I'd like us to introduce a .git-blame-ignore-revs file to our repositories to help git blame skip reformatting commits.

Something like:

mr run sh -c "git log --reverse --grep 'black formatting' --grep 'Enable black' --grep 'Reformat code' --pretty=format:'# %s%n%H%n' > .git-blame-ignore-revs"
mr run git add .git-blame-ignore-revs
mr run git commit -m "Add .git-blame-ignore-revs file with automatic reformatting commits"

In D7502#196088, @olasd wrote:

I thought we could introduce flake8-bugbear for line length limits, as suggested by black's docs: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length, but I can't seem to find a way to *only* enable B950 and not the other bugbear warnings. sigh.

(And, obviously, most modules hit a lot of the bugbear warnings. Which are, probably, mostly sensible. But it's still a bunch of unrelated work.)

Instead of running black swh, I would suggest using pre-commit run -a black to make sure the proper version is used, and all affected files are properly reformatted (we have some python files not in the swh directory).

I have upgraded black in my venv to its latest version but did not think about Python files outside the swh directory, thanks for noticing.

I've tried this out using mr (I'm not sure why the mass-change script is needed, mr run works just fine, and gives us plenty of chances to inspect the changes instead of yolo-pushing everything!):

TIL about mr run, seems way more simpler to use than the change-all-repos script. For the record, I commented the git push instruction in it as I already had some bad surprises with it.

I thought we could introduce flake8-bugbear for line length limits, as suggested by black's docs: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length, but I can't seem to find a way to *only* enable B950 and not the other bugbear warnings. sigh.

I forgot to run make check again, good catch ! Indeed using flake8-bugbear seems the best way to proceed regarding line length warnings.
I managed to activate only that warning using the following flake8 configuration:

[flake8]
max-line-length = 88
# E203: whitespaces before ':' <https://github.com/psf/black/issues/315>
# E231: missing whitespace after ','
# E501: line too long, use B950 warning from flake8-bugbear instead
# W503: line break before binary operator <https://github.com/psf/black/issues/52>
ignore = E203,E231,E501,W503
select = C,E,F,W,B950

To activate flake8-bugbear with tox, I used that configuration in tox.ini:

[testenv:flake8]
skip_install = true
deps =
  flake8
  flake8-bugbear
commands =
  {envpython} -m flake8 \
    --exclude=.tox,.git,__pycache__,.eggs,*.egg,node_modules

And to activate its use with pre-commit, I used that configuration in .pre-commit-config.yaml:

repos:
  - repo: https://gitlab.com/pycqa/flake8
    rev: 4.0.1
    hooks:
      - id: flake8
        additional_dependencies: [flake8-bugbear]

Finally, I'd like us to introduce a .git-blame-ignore-revs file to our repositories to help git blame skip reformatting commits.

I did not know that git feature, awesome !

In D7502#196144, @anlambert wrote:

I thought we could introduce flake8-bugbear for line length limits, as suggested by black's docs: https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length, but I can't seem to find a way to *only* enable B950 and not the other bugbear warnings. sigh.

I forgot to run make check again, good catch ! Indeed using flake8-bugbear seems the best way to proceed regarding line length warnings.
I managed to activate only that warning

Very nice!

using the following flake8 configuration:

[flake8]
max-line-length = 88
# E203: whitespaces before ':' <https://github.com/psf/black/issues/315>
# E231: missing whitespace after ','
# E501: line too long, use B950 warning from flake8-bugbear instead
# W503: line break before binary operator <https://github.com/psf/black/issues/52>
ignore = E203,E231,E501,W503
select = C,E,F,W,B950

Are E203, E231 and W503 still needed with the most recent black style?

My attempts at configuring bugbear focused on the (recent-ish, flake8 4.x) extend-ignore and extend-select options, to avoid having to repeat the default settings for them. I guess they interact poorly with the way bugbear hooks into flake8.

Considering that we'll have to poke at that config everywhere anyway the next time we upgrade flake8, we may as well do it that way...

To activate flake8-bugbear with tox, I used that configuration in tox.ini:

[testenv:flake8]
skip_install = true
deps =
  flake8
  flake8-bugbear
commands =
  {envpython} -m flake8 \
    --exclude=.tox,.git,__pycache__,.eggs,*.egg,node_modules

We could move this node_modules exclude to the setup.cfg (adding extend-exclude = node_modules to the flake8 section). This would avoid having to repeat the default excludes.

And to activate its use with pre-commit, I used that configuration in .pre-commit-config.yaml:

repos:
  - repo: https://gitlab.com/pycqa/flake8
    rev: 4.0.1
    hooks:
      - id: flake8
        additional_dependencies: [flake8-bugbear]

Yeah, that's what I had too. Seeing that 4.0.1 makes me think we should maybe pin the flake8 version in the tox config too.

Finally, I'd like us to introduce a .git-blame-ignore-revs file to our repositories to help git blame skip reformatting commits.

I did not know that git feature, awesome !

We still need to use it manually (with --ignore-revs-file), but it's somewhat standard to have this file available now.

Are E203, E231 and W503 still needed with the most recent black style?

My attempts at configuring bugbear focused on the (recent-ish, flake8 4.x) extend-ignore and extend-select options, to avoid having to repeat the default settings for them. I guess they interact poorly with the way bugbear hooks into flake8.

Considering that we'll have to poke at that config everywhere anyway the next time we upgrade flake8, we may as well do it that way...

As we use the ignore configuration entry and not the extend-ignore (which indeed does not play well with flake8-bugbear), these warnings still need to be explicitly ignored.

We could move this node_modules exclude to the setup.cfg (adding extend-exclude = node_modules to the flake8 section). This would avoid having to repeat the default excludes.

Ack, I will handle that in a separate diff.

Yeah, that's what I had too. Seeing that 4.0.1 makes me think we should maybe pin the flake8 version in the tox config too.

It seems a good idea to have pre-commit and tox configuration in sync indeed.

We still need to use it manually (with --ignore-revs-file), but it's somewhat standard to have this file available now.

github and gitlab have plans to automatically support that feature in blame views if the file is present, github seems to advance well on the subject.

Otherwise I came up with a script to automate the black upgrade in all our repositories, it can be found in T3922#82548.