Blacken the codebase #8903

pradyunsg · 2020-09-23T14:43:19Z

Follow up to #8803

_{Note to self: how to update this if it stalls}

$ git rebase -i master
# (pick the first 3 commits)
$ nox -s lint
$ for file in $(git ls-files --modified); do
..    echo -e "\033[32m$file\033[0m"
..    git add $file
..    git commit -m "Blacken $file"
.. done
$ git cherry-pick 9443e6f

pfmoore · 2020-09-23T15:05:29Z

Ah, this is the one that's probably got things in it I'm actually going to hate. I'm not even going to look at it, as it'll probably annoy me too much. I thought #8902 included this, which is why I was "pleasantly surprised" it was mostly OK.

I'm not going to argue against us adopting black, that's a huge debate that I don't have the energy or interest in starting again, so I'll push my head firmly into the sand here.

pradyunsg · 2020-09-23T15:20:55Z

Confirmed that the concerns that caused issues w/ trailing commas in #8803, are indeed resolved in this one.

noxfile.py

pradyunsg · 2020-09-23T16:13:10Z

Uhh... @pypa-bot hasn't reported the NEWS fragment stuff. Oh no.

uranusjr · 2020-09-23T16:41:02Z

Can we do this a module at a time instead? Black is known to not always make the best format decision (sometimes vastly incorrect IMO), and I don’t want the lost context get buried in such a hugh changeset when it does make mistakes.

pradyunsg · 2020-09-23T16:54:38Z

Can we do this a module at a time instead?

Do you want me to make separate commits for each file?

webknjaz · 2020-09-23T17:02:20Z

noxfile.py

-            "docs/" + kind,
-            "docs/build/" + kind,
+            "-c=docs/html",  # see note above
+            f"-d=docs/build/doctrees/{kind}",


Is it me or this feels like a hack?

Using f-strings feels like a hack? I was going to do this anyway, and black forcing my move to be sooner is OK with me. 🤷

not f-strings, putting two pieces into one item

Agreed, I feel that we shouldn't try to fight the tool like this. It's up to black to have a way to allow us to express our intent here, which is to have two arguments, "-c" and "docs/html" which are separate but related. That's the semantics of the command line we'd normally write in the shell, and we should be able to reflect that readably in code.

IMO we just add #fmt: off to signal that black doesn't do what we want (see here)

Yes, this is exactly what I meant

nox is Python 3.6+, and f-strings are awesome. :)

pradyunsg · 2020-09-23T17:16:51Z

Okay, GitHub does not like it when there's more than 250 commits. I'mma squash all the tests/data/ commits into a single one.

pfmoore · 2020-09-23T18:13:36Z

Do you want me to make separate commits for each file?

Personally, my "unit of review" is the PR, not the commit, so this would not help for me. I'd rather see multiple PRs. I've opted out of getting involved in reviewing the black formatting changes, though, so my opinion isn't relevant.

pradyunsg

black is not perfect, but I still think it'll be worth it for us to adopt it, because it's consistent and gives a good output in nearly all cases. In many of the cases, black's "ugly" output is a good indicator of you're doing too much and that's IMO a good thing.

Unsuprisingly, I like it. It is also extremely frustrating when it makes the wrong choices though.

pradyunsg · 2020-09-23T17:19:39Z

src/pip/_internal/cache.py

-            not canonical_package_name or
-            not link
-        )
+        can_not_cache = not self.cache_dir or not canonical_package_name or not link


Looks like psf/black#449, so yay!

pradyunsg · 2020-09-23T17:21:01Z

src/pip/_internal/cli/cmdoptions.py

    sdist_dependencies_allowed = (
-        options.format_control != binary_only and
-        not options.ignore_dependencies
+        options.format_control != binary_only and not options.ignore_dependencies


There's no way a robot would guess the readability semantics you were after...

Which is fine? TBH the point is for it to do a good-enough job. :)

Depends on the definition of fine

I'm starting to become a strong supporter of #fmt: off 🙁

Again, that's fine -- black isn't going to be perfect everywhere. The point is that it's good enough almost all the time and the consistency is very useful.

To me, # fmt: off is an indication that: I'm sure that this is better than what black did for everyone that'll read the code. This is a great place to use it.

pradyunsg · 2020-09-23T17:24:11Z

src/pip/_internal/commands/freeze.py

            help="Use the order in the given requirements file and its "
-                 "comments when generating output. This option can be "
-                 "used multiple times.")
+            "comments when generating output. This option can be "
+            "used multiple times.",


Oh come on.

My favorite is when it does ('str 1' 'str2' 'str 3') and nobody cares

It no longer does that. :)

psf/black#26

You can use https://github.com/keisheiled/flake8-implicit-str-concat to find stuff like 'str 1' 'str2' 'str 3'

(None currently found on this branch)

@hugovk implicit str concat is not bad if formatted properly

@webknjaz flake8-implicit-str-concat only prohibits implicit string concatenation on one line

pradyunsg · 2020-09-23T17:24:33Z

src/pip/_internal/commands/freeze.py

+            help="If in a virtualenv that has global access, do not output "
+            "globally-installed packages.",


Yea, okay, this needs to be fixed throughout the codebase actually.

If by "fixed" you mean "make black not do that" then I agree 🙂

It needs us to parenthesize all these help assignments, to make them consistent throughout, which is an overall positive change IMO. :)

I personally prefer this form but I can see how it could be confusing w/o parentheses.

pradyunsg · 2020-09-23T17:25:53Z

src/pip/_internal/commands/install.py

+            parts.extend(
+                [
+                    user_option_part,
+                    " or ",
+                    permissions_part.lower(),
+                ]
+            )


I really wouldn't write it like this, but I guess an extra variable doesn't hurt.

Looks like psf/black#925, so it'll get fixed down the line?

pradyunsg · 2020-09-23T17:59:46Z

src/pip/_internal/req/req_uninstall.py

+        if dist_path in {
+            p
+            for p in {sysconfig.get_path("stdlib"), sysconfig.get_path("platstdlib")}
+            if p
+        }:


We should simplify this by having a variable for the RHS:

paths = {p for p in [sysconfig.get_path("stdlib"), sysconfig.get_path("platstdlib")] if p}

Actually that whole construct is just opaque IMO. Needs rewriting.

pradyunsg · 2020-09-23T18:00:11Z

src/pip/_internal/resolution/legacy/resolver.py

+        self._discovered_dependencies = defaultdict(
+            list
+        )  # type: DiscoveredDependencies


I blame Python 2 (type comments) and over-long variable names for that.

pradyunsg · 2020-09-23T18:02:30Z

src/pip/_internal/resolution/resolvelib/factory.py

-        msg = msg + "\n\n" + \
-            "To fix this you could try to:\n" + \
-            "1. loosen the range of package versions you've specified\n" + \
-            "2. remove package versions to allow pip attempt to solve " + \
-            "the dependency conflict\n"
+        msg = (
+            msg
+            + "\n\n"
+            + "To fix this you could try to:\n"
+            + "1. loosen the range of package versions you've specified\n"
+            + "2. remove package versions to allow pip attempt to solve "
+            + "the dependency conflict\n"
+        )


I'd probably do implicit concat + str.format

No, because runtime concat would be happening multiple times which is slower + a bunch of implicitly concatenated strings parsed as one literal and then formatted just once...

pradyunsg · 2020-09-23T18:12:45Z

src/pip/_internal/utils/packaging.py

+    if isinstance(dist, pkg_resources.DistInfoDistribution) and dist.has_metadata(
+        metadata_name
+    ):


pradyunsg · 2020-09-23T18:14:36Z

src/pip/_internal/vcs/git.py

+            newpath = initial_slashes + urllib_request.url2pathname(path).replace(
+                "\\", "/"
+            ).lstrip("/")


Uhm... really?

pradyunsg · 2020-09-23T18:31:38Z

Personally, my "unit of review" is the PR, not the commit, so this would not help for me. I'd rather see multiple PRs.

Ah, smart. I think I can do this incrementally too. :)

I've opted out of getting involved in reviewing the black formatting changes, though, so my opinion isn't relevant.

:)

pradyunsg · 2020-09-23T18:33:25Z

Making this PR a draft again, and I'll make a bunch of smaller PRs for adding black in. :)

BrownTruck · 2021-02-04T16:01:48Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

pradyunsg · 2021-03-05T16:44:28Z

Closing since #8543 tracks this.

pradyunsg added skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes labels Sep 23, 2020

pradyunsg mentioned this pull request Sep 23, 2020

Linter updates #8902

Merged

pradyunsg marked this pull request as ready for review September 23, 2020 15:19

webknjaz reviewed Sep 23, 2020

View reviewed changes

noxfile.py Outdated Show resolved Hide resolved

pradyunsg removed the skip news Does not need a NEWS file entry (eg: trivial changes) label Sep 23, 2020

pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Sep 23, 2020

pradyunsg added 3 commits September 23, 2020 21:47

Prepare flake8 for black

a00f85d

Prepare isort for black

58c594c

Add linter: black

be189c5

pradyunsg changed the title ~~Linter updates with black~~ Blacken the codebase Sep 23, 2020

pradyunsg force-pushed the linter-updates-with-black branch from ddc92c8 to 0e3750f Compare September 23, 2020 16:59

webknjaz reviewed Sep 23, 2020

View reviewed changes

pradyunsg added 12 commits September 23, 2020 22:35

Blacken docs/html/conf.py

917b41d

Blacken docs/pip_sphinxext.py

ed383dd

Blacken noxfile.py

228405e

Blacken setup.py

f477a9f

Blacken src/pip/__main__.py

e59ba23

Blacken src/pip/_internal/build_env.py

d7013db

Blacken src/pip/_internal/cache.py

30e9ffa

Blacken src/pip/_internal/cli/autocompletion.py

8341d56

Blacken src/pip/_internal/cli/base_command.py

3d3461e

Blacken src/pip/_internal/cli/cmdoptions.py

d489b0f

Blacken src/pip/_internal/cli/command_context.py

591fe48

Blacken src/pip/_internal/cli/main.py

9265b28

pradyunsg added 14 commits September 23, 2020 22:46

Blacken tests/unit/test_utils_pkg_resources.py

26f5afa

Blacken tests/unit/test_utils_subprocess.py

4e02578

Blacken tests/unit/test_utils_temp_dir.py

c0831dd

Blacken tests/unit/test_utils_unpacking.py

f06519d

Blacken tests/unit/test_utils_virtualenv.py

1e6c84a

Blacken tests/unit/test_utils_wheel.py

fd62a58

Blacken tests/unit/test_vcs.py

83744bd

Blacken tests/unit/test_vcs_mercurial.py

56dbc68

Blacken tests/unit/test_wheel.py

69c573d

Blacken tests/unit/test_wheel_builder.py

8581efe

Blacken tests/yaml/linter.py

24227fa

Blacken tools/automation/release/__init__.py

c3b6037

Blacken tools/tox_pip.py

c7b20fb

Use f-strings in noxfile.py

a96d51d

nox is Python 3.6+, and f-strings are awesome. :)

pradyunsg force-pushed the linter-updates-with-black branch from 9443e6f to a96d51d Compare September 23, 2020 17:17

pradyunsg commented Sep 23, 2020

View reviewed changes

pradyunsg mentioned this pull request Sep 23, 2020

Prepare other linters for black #8906

Merged

pradyunsg marked this pull request as draft September 23, 2020 18:32

pradyunsg added this to the The Blackening milestone Sep 23, 2020

pradyunsg mentioned this pull request Sep 23, 2020

Adopting black (and linter updates!) #8543

Closed

BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 4, 2021

pradyunsg closed this Mar 5, 2021

pradyunsg deleted the linter-updates-with-black branch March 5, 2021 16:44

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2021

		help="If in a virtualenv that has global access, do not output "
		"globally-installed packages.",

Blacken the codebase #8903

Blacken the codebase #8903

Conversation

pradyunsg commented Sep 23, 2020 • edited

pfmoore commented Sep 23, 2020

pradyunsg commented Sep 23, 2020

pradyunsg commented Sep 23, 2020

uranusjr commented Sep 23, 2020

pradyunsg commented Sep 23, 2020

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

pradyunsg commented Sep 23, 2020

pfmoore commented Sep 23, 2020

pradyunsg left a comment • edited

Choose a reason for hiding this comment

pradyunsg Sep 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg Sep 23, 2020 • 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

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

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

pradyunsg commented Sep 23, 2020

pradyunsg commented Sep 23, 2020

BrownTruck commented Feb 4, 2021

pradyunsg commented Mar 5, 2021 • edited

pradyunsg commented Sep 23, 2020 •

edited

pradyunsg left a comment •

edited

pradyunsg Sep 23, 2020 •

edited

pradyunsg Sep 23, 2020 •

edited

pradyunsg commented Mar 5, 2021 •

edited