Use black for code formatting #6264

hexagonrecursion · 2022-02-08T06:16:36Z

https://github.com/psf/black is an The uncompromising Python code formatter.

Examples of things black does that I know we want:

diff --git a/x.py b/x.py
index 753b8f0..1532915 100644
--- a/x.py
+++ b/x.py
@@ -1,7 +1,8 @@
 missing_trailing_comma = {
     "foo": "Consequatur in omnis eius aut est aut similique ipsa.",
-    "bar": "Dolorem blanditiis deleniti excepturi."
+    "bar": "Dolorem blanditiis deleniti excepturi.",
 }

+
 def triple_single_quotes():
-    '''Moo'''
+    """Moo"""

This commit only applies the formatting to demo it.
If you like what you see, I'll add back to tox.ini and to CI.

codecov-commenter · 2022-02-08T09:40:25Z

Codecov Report

Merging #6264 (c0b2008) into master (78f0414) will decrease coverage by 0.02%.
The diff coverage is 77.23%.

@@            Coverage Diff             @@
##           master    #6264      +/-   ##
==========================================
- Coverage   82.71%   82.68%   -0.03%     
==========================================
  Files          39       39              
  Lines       10525    10525              
  Branches     2076     2076              
==========================================
- Hits         8706     8703       -3     
- Misses       1312     1314       +2     
- Partials      507      508       +1

Impacted Files	Coverage Δ
src/borg/archiver.py	`78.42% <ø> (ø)`
src/borg/helpers/checks.py	`40.74% <0.00%> (ø)`
src/borg/helpers/errors.py	`100.00% <ø> (ø)`
src/borg/lrucache.py	`100.00% <ø> (ø)`
src/borg/platform/__init__.py	`84.00% <ø> (ø)`
src/borg/version.py	`100.00% <ø> (ø)`
src/borg/xattr.py	`51.19% <ø> (ø)`
src/borg/selftest.py	`76.08% <20.00%> (ø)`
src/borg/logger.py	`69.74% <44.00%> (ø)`
src/borg/helpers/time.py	`82.19% <54.54%> (ø)`
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78f0414...c0b2008. Read the comment docs.

hexagonrecursion · 2022-02-08T14:32:21Z

This causes CodeQL to fail:

print(
    'Your passphrase (between double-quotes): "%s"' % passphrase,
    file=sys.stderr,
)

error: Clear-text logging of sensitive information

Not a bug: a deliberate feature.

ThomasWaldmann · 2022-02-08T14:39:16Z

I like a lot of the changes black does, but (sadly) can't agree with it completely. So, guess no black for now.

IIRC i have already checked that line (false positive) due to codeql feedback, but likely it popped up now because the line was reformatted by black.

hexagonrecursion · 2022-02-08T14:47:30Z

can't agree with it completely

Could you please provide more details? There is a chance black can be configured to suit your tastes. If not, perhaps we could try a more configurable formatter. It would be useful to know more about your stylistic preferences when "shopping" for a it.

ThomasWaldmann

commented some black changes.

my main issue is black using too much vspace (and too little hspace).

our screens usually have more width than height (like 16:9 or 16:10), but black is transforming a lot of hspace usage into vspace usage.

for the same reason, we do not use the "80 chars wide" pep8 recommendation, but more.

it is important that a reasonable amount of code fits into the screen / into the editor window or one ends up with having to scroll way too often.

conftest.py

docs/conf.py

setup.py

setup_compress.py

setup_docs.py

hexagonrecursion · 2022-02-08T15:09:58Z

Thank you

hexagonrecursion · 2022-02-08T15:16:09Z

Good news: line length is configurable in black. The default is 88. Iet's see what --line-length=100 looks like.

hexagonrecursion · 2022-02-08T15:49:06Z

--line-length=120 seams to address most of your vspace concerns.

ThomasWaldmann · 2022-02-08T16:40:14Z

https://github.com/borgbackup/borg/blob/master/setup.cfg#L91 yeah, 120 is what we have there also.

ThomasWaldmann

much better, some dislikes remaining.

but we get closer to the point where i'ld say automated blackening has more advantages than disadvantages.

we can't merge this now though, this must wait until after 1.2.0 (not sure whether it should be limited to master then and not be done in 1.2-maint. while that would be more conservative, it would make backports harder, guess we do not want that).

in any case, the first run of black needs to be a totally separate PR with no manual changes.

ThomasWaldmann · 2022-02-08T16:45:37Z

src/borg/constants.py

+    [
+        "path",
+        "source",
+        "rdev",
+        "chunks",
+        "chunks_healthy",
+        "hardlink_master",
+        "mode",
+        "user",
+        "group",
+        "uid",
+        "gid",
+        "mtime",
+        "atime",
+        "ctime",
+        "birthtime",
+        "size",
+        "xattrs",
+        "bsdflags",
+        "acl_nfs4",
+        "acl_access",
+        "acl_default",
+        "acl_extended",
+        "part",
+    ]


strange. a lot of vspace usage is gone, but here another extreme one is still present.

Not strange: black either puts each item on its own line or puts them all on the same line. This is too long to fit in 120 characters so black puts each item on a separate line.

black respects # fmt: off/on to suppress formatting in special situations. https://stackoverflow.com/a/58584557

ah, that's nice to deal with a few cases.

ThomasWaldmann · 2022-02-08T16:52:52Z

to check:

what about cython? black can't do that, right?
are all 3rd party code parts excluded?
how likely is it that blackening accidentally introduces semantic code changes / bugs?

RonnyPfannschmidt · 2022-02-08T21:02:01Z

psf/black#359 is incomplete

semantic changes are extremely unlikely

hexagonrecursion · 2022-02-09T06:15:53Z

this must wait until after 1.2.0

Ok. I'll get back to this when 1.2.0 comes out.

hexagonrecursion · 2022-02-09T06:27:10Z

src/borg/constants.py

+REQUIRED_ITEM_KEYS = frozenset(
+    [
+        "path",
+        "mtime",


There was a trailing comma after mtime. This would have stayed on one line:

REQUIRED_ITEM_KEYS = frozenset(['path', 'mtime'])

I like trailing commas, they make future changes more "regular".

It appears that trailing commas force black to split items across multiple lines:

REQUIRED_ITEM_KEYS = frozenset(['path', 'mtime',]) REQUIRED_ITEM_KEYS = frozenset(['path', 'mtime'])

becomes:

REQUIRED_ITEM_KEYS = frozenset( [ "path", "mtime", ] ) REQUIRED_ITEM_KEYS = frozenset(["path", "mtime"])

Nope. This is a feature, but one that can be disabled. See psf/black#1824

pipx run black .

produces:

diff --git a/example.py b/example.py index 6e257e0..ed2a7da 100644 --- a/example.py +++ b/example.py @@ -1,2 +1,7 @@ -REQUIRED_ITEM_KEYS = frozenset(["path", "mtime",]) +REQUIRED_ITEM_KEYS = frozenset( + [ + "path", + "mtime", + ] +) REQUIRED_ITEM_KEYS = frozenset(["path", "mtime"])

but:

pipx run black --skip-magic-trailing-comma .

produces:

diff --git a/example.py b/example.py index 6e257e0..3a4ce71 100644 --- a/example.py +++ b/example.py @@ -1,2 +1,2 @@ -REQUIRED_ITEM_KEYS = frozenset(["path", "mtime",]) +REQUIRED_ITEM_KEYS = frozenset(["path", "mtime"]) REQUIRED_ITEM_KEYS = frozenset(["path", "mtime"])

and what if we want to just keep the trailing comma and not have it split into one line per element?

it looks like the idea of "always do the same change" when adding or removing an element is not present in black:

[] # start with an empty list [first, ] # add first element [first, second, ] # add another element - same procedure as first element.

btw, for tuples, this also solves the non-tupleness of (first), because then it is (first, ).

I am sorry, but black does not have an option to keep a trailing comma when putting the output on a single-line. Is this a deal-breaker? We could theoretically ask for such an option to be added, but I am not sure it'll happen given that the default is magic trailing comma - a trailing comma indicates that the output should be split across multiple lines.

i actually like the fact that the comma is used as indicator on how one wants to be diff friendly/lined up

with the tailing comma black prepares multi-line, without the comma black prepares single line

not really a deal-breaker, just one item on the disadvantages list.

enkore · 2022-02-14T21:42:44Z

If something like this is done (personally not a fan) I'd suggest adding a blame-ignore-revs file and a note to the developer's docs to set that up after cloning so that git blame doesn't become completely useless.

ThomasWaldmann · 2022-02-20T15:59:59Z

https://github.com/ipython/ipython/pull/12091/files see there for an example of what @enkore meant.

ThomasWaldmann · 2022-02-24T21:32:19Z

@hexagonrecursion could you please:

rebase on current master
have 1 or a few changesets that integrate black (see also previous comment)
have a separate changeset that shows how black changes the source when using the integration

This blocks borgbackup#6264 because black configuration is stored in pyproject.toml. See <https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#configuration-format> I copied this from <https://setuptools.pypa.io/en/latest/userguide/quickstart.html#basic-use>

hexagonrecursion · 2022-02-25T05:56:36Z

Saving a link here for my future self: Avoiding ruining git blame

enkore · 2022-02-25T16:07:57Z

I'd suggest to pin Black to a 22.x version because the FAQ of Black points out ("How stable is Black’s style?") that the output isn't stable across major (yearly) versions,

ThomasWaldmann · 2022-02-25T19:46:51Z

@enkore good idea. otoh, i would suspect that changes would be rather minor maybe?

ThomasWaldmann · 2022-02-27T17:57:46Z

could be rebased in current master now and soon be applied to master.

we'll need to keep master and 1.2-maint separated already, so IF we apply to 1.2-maint also, we would hope that the result of the transformation is about the same as in master.

hexagonrecursion · 2022-03-01T06:16:56Z

could be rebased in current master now and soon be applied to master.

we'll need to keep master and 1.2-maint separated already, so IF we apply to 1.2-maint also, we would hope that the result of the transformation is about the same as in master.

I'll do that after Add pyproject.toml #6355 is merged

hexagonrecursion · 2022-03-02T04:28:09Z

security: do not trust me! It is easy to bury a backdoor in a diff this big. Run black yourself and compare.

Even if you trust me, there is always a chance that my machine is compromised. Why risk?

ThomasWaldmann

thanks for your note about security, you're very careful.

.git-blame-ignore-revs

hexagonrecursion · 2022-03-05T05:51:29Z

@ThomasWaldmann I am confused. GitHub tells me that you requested changes, but in your most recent review (Mar 2) you only commented about GitHub not respecting .git-blame-ignore-revs. Do you want me to change something?

ThomasWaldmann · 2022-03-05T06:09:01Z

@hexagonrecursion No, nothing to do here for you.

Next step is me finishing this.

hexagonrecursion · 2022-03-22T05:37:59Z

Rebased on current master

hexagonrecursion · 2022-03-31T10:55:54Z

.git-blame-ignore-revs is now supported by github https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

ThomasWaldmann · 2022-07-06T14:03:33Z

Superseded by #6842.

ThomasWaldmann requested changes Feb 8, 2022

View reviewed changes

hexagonrecursion force-pushed the black branch from 015cf83 to 236d2d9 Compare February 8, 2022 15:46

hexagonrecursion requested a review from ThomasWaldmann February 8, 2022 15:49

ThomasWaldmann reviewed Feb 8, 2022

View reviewed changes

hexagonrecursion commented Feb 9, 2022

View reviewed changes

ThomasWaldmann mentioned this pull request Feb 20, 2022

use black for code formatting #6327

Closed

hexagonrecursion marked this pull request as draft February 25, 2022 03:32

hexagonrecursion mentioned this pull request Feb 25, 2022

Add pyproject.toml #6355

Merged

hexagonrecursion force-pushed the black branch from 93439e5 to cf12e47 Compare March 2, 2022 03:40

hexagonrecursion marked this pull request as ready for review March 2, 2022 04:05

hexagonrecursion requested a review from ThomasWaldmann March 2, 2022 04:06

ThomasWaldmann reviewed Mar 2, 2022

View reviewed changes

.git-blame-ignore-revs Outdated Show resolved Hide resolved

hexagonrecursion added 3 commits March 22, 2022 08:32

black integration

a564b4b

pipx run black .

23ab437

Add .git-blame-ignore-revs

c0b2008

hexagonrecursion force-pushed the black branch from cf12e47 to c0b2008 Compare March 22, 2022 05:37

ThomasWaldmann added this to the 2.0.0a4 milestone Jul 5, 2022

ThomasWaldmann closed this Jul 6, 2022

Use black for code formatting #6264

Use black for code formatting #6264

Conversation

hexagonrecursion commented Feb 8, 2022

codecov-commenter commented Feb 8, 2022 • edited

Codecov Report

hexagonrecursion commented Feb 8, 2022

ThomasWaldmann commented Feb 8, 2022 • edited

hexagonrecursion commented Feb 8, 2022

ThomasWaldmann left a comment • edited

Choose a reason for hiding this comment

hexagonrecursion commented Feb 8, 2022

hexagonrecursion commented Feb 8, 2022

hexagonrecursion commented Feb 8, 2022

ThomasWaldmann commented Feb 8, 2022

ThomasWaldmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasWaldmann commented Feb 8, 2022 • edited

RonnyPfannschmidt commented Feb 8, 2022

hexagonrecursion commented Feb 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enkore commented Feb 14, 2022 • edited

ThomasWaldmann commented Feb 20, 2022

ThomasWaldmann commented Feb 24, 2022

hexagonrecursion commented Feb 25, 2022

enkore commented Feb 25, 2022 • edited

ThomasWaldmann commented Feb 25, 2022

ThomasWaldmann commented Feb 27, 2022

hexagonrecursion commented Mar 1, 2022

hexagonrecursion commented Mar 2, 2022 • edited

ThomasWaldmann left a comment

Choose a reason for hiding this comment

hexagonrecursion commented Mar 5, 2022

ThomasWaldmann commented Mar 5, 2022

hexagonrecursion commented Mar 22, 2022

hexagonrecursion commented Mar 31, 2022

ThomasWaldmann commented Jul 6, 2022

codecov-commenter commented Feb 8, 2022 •

edited

ThomasWaldmann commented Feb 8, 2022 •

edited

ThomasWaldmann left a comment •

edited

ThomasWaldmann commented Feb 8, 2022 •

edited

hexagonrecursion commented Feb 9, 2022 •

edited

enkore commented Feb 14, 2022 •

edited

enkore commented Feb 25, 2022 •

edited

hexagonrecursion commented Mar 2, 2022 •

edited