Format code with Black

[hugovk]

Continuation of #5425.

Adds Black config in pyproject.toml:

• Exclude _vendor dir
• No other options, uses Black's defaults

Update isort config in setup.cfg for compatibility with Black:

• Requires the new isort option ensure_newline_before_comments so isort plays nicely with Black
• It's not been released yet, so update .pre-commit-config.yaml to use the latest isort develop branch SHA for the time being
• Black issue: isort removes newlines before standalone comments if the following line is an import of the same group psf/black#251
• isort PR: Add configuration to ensure a newline before each comment PyCQA/isort#1000

[hugovk]

About the new isort option needed, @timothycrosley says of the next isort release:

I can get it scheduled comfortably for 2 weeks for now. if it is really urgent I can try to get the release in good shape within a week. I hate to block the work on pip!

PyCQA/isort#1000 (comment)

What sort of urgency do we have here? Can we use the dev version for a couple of weeks?

[pfmoore]

IMO, there's really no rush on this change. We can simply wait for a new version of isort. I see very little reason for haste here.

[gaborbernat]

can't we wait for a. release?

[hugovk]

Yep, I can update this when it's out.

[pradyunsg]

I chimed in over on the isort thread to clarify that there's no pressure from our end, for a quicker release of isort.

I've marked this as blocked since, well, we're blocked to merge, on that release.

Beyond that, I feel there's a few adoption related things we should figure out but my headache means I'm going to go sleep and not think too much about this.

[pradyunsg]

Also also, thanks for filing this PR @hugovk! Much appreciated!

[pfmoore]

Things that come to mind for me, as worth reviewing before merging.

• Are there any other 3rd party issues that need fixing before we merge? (There were some black issues at the time of the original PR, but I assume they are all fixed by now).
• A major reformatting like this will probably cause merge conflicts for most other PRs. Do we need to do anything to manage the impact of that?
• Are there any style issues we really don't want to put up with, and if so can we set config options to change black's behaviour? (Remembering that the whole point of this PR is to avoid debating style, so "let's go with black, but change stuff" is a rather self-contradictory position to take...)
• Have black's rules changed over time, and do we therefore need to specify a minimum version for contributors to use (or is "reasonably up to date" sufficient)?

I won't review any of these myself, as I think I'd get sucked into the "I don't like the style choices" cycle again, and I don't want to see this PR blocked on that.

I doubt any of these are a big deal in practice.

[pradyunsg]

Are there any other 3rd party issues that need fixing before we merge? (There were some black issues at the time of the original PR, but I assume they are all fixed by now).

I think so, yea.

A major reformatting like this will probably cause merge conflicts for most other PRs. Do we need to do anything to manage the impact of that?

Uhm... One of us can go in and drop a comment on these that -- hey, this is happening because of XYZ, here's how you can fix that.

I don't know how necessary that is though. :)

Are there any style issues we really don't want to put up with, and if so can we set config options to change black's behaviour?

I mean, there's style choices I disagree with. :P

More importantly though, target-version will definitely be py27 (determines which syntax features can be used), so we're gonna have configuration none the less.

Given that, I'd like it if we "skip-string-normalization", mostly because that's gonna cause a lot more churn than I feel it'd add value for. But it's cool if we don't.

Have black's rules changed over time, and do we therefore need to specify a minimum version for contributors to use (or is "reasonably up to date" sufficient)?

I think we don't need a minimum version to be listed somewhere -- as long as they're on the same version as listed in the pre-commit configuration, we should be fine. Regarding the changes in formatting, the README says:

until the formatter becomes stable, you should expect some formatting to change in the future. That being said, no drastic stylistic changes are planned, mostly responses to bug reports.

[pradyunsg]

@hugovk Since you're likely gonna need to update this PR later, I have a small request: could you make all the "configure tooling" commits first, then a big "Format with Black", and then smaller fixes in the formatted black code?

IMO, ideally, we should only have a single "Format with Black" commit. :)

[chrahunt]

I'm -1 on skip-string-normalization. IMO the extra changes this one time are worth never having to think about it again. I think it would be a good idea to have the string normalization in a separate commit so it's easier to review the changes which could have some impact (e.g. pypa/packaging#196) without the excess noise.

[pypa-bot]

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 eligible for code review and hopefully merging!

[hugovk]

Black bug with --target-version py27

More importantly though, target-version will definitely be py27 (determines which syntax features can be used), so we're gonna have configuration none the less.

Sure. There's a bug (psf/black#768) in the latest Black 19.3b0 release with --target-version py27. On clean pip master:

$ black --version
black, version 19.3b0
$ black --target-version py27 src/pip/_internal/cli/base_command.py
error: cannot format src/pip/_internal/cli/base_command.py: Cannot parse: 168:58:             print("ERROR: Pipe to stdout was broken", file=sys.stderr)
All done! 💥 💔 💥
1 file failed to reformat.

It's fixed with the Black master branch:

$ black --version
black, version 19.3b1.dev89+g0acad54
$ black --target-version py27 src/pip/_internal/cli/base_command.py
reformatted src/pip/_internal/cli/base_command.py
All done! ✨ 🍰 ✨
1 file reformatted.

There's also psf/black#752, also fixed in master, which does this:

-        print("Creating %s" % output)
+        print ("Creating %s" % output)

I've no idea when the next Black release (beta or otherwise) is due out. There was going to be one earlier in the year but it's not happened yet:

• Non-prerelease black release psf/black#517
• New Release To Fix 2.7 psf/black#988

[pfmoore]

The bug that formats

x = ("a"
       "b")

as

x = "a" "b"

is still present, too. That hits us in a few places, I believe - and the unnecessary one-line concatenation is pretty bad for readability.

Regarding skip-string-normalization, I'm -1 on having this in our config. I personally dislike the rule black uses (why force only one quote type to be used when Python provides two for flexibility in the first place?) but the entire point of using black seems to me to be to avoid this type of debate, so if we can't accept string normalisation, what's the point in this PR?

[hugovk]

Yep, I can manually fix those one-off "a" "b" in this (and have done some already).

[pfmoore]

Hmm, I'm -1 on this being something we have to manually fix as an ongoing thing. Again, IMO if what black generates isn't what we want, then it seems like we're just swapping one version of style policing for a different one. I don't think fixing lint checks like we currently do is so onerous that it needs "improving", so I see black's advantage as basically being that it can eliminate it altogether.

But whatever - I promised myself I wouldn't get sucked into this again, so that's my opinion, I'm not going to fight if others are OK with the situation as it is.

[hugovk]

I think this will be rare once the initial sweep is done and Black won't undo the change.

(Here's the Black issue: psf/black#26)

[pradyunsg]

Hmm, I'm -1 on this being something we have to manually fix as an ongoing thing

Ditto.

I'm hoping we can find/make a pre-commit linter for catching this. That way even when black does spew this out, we'd catch it in that different linter. (benefits of pre-commit yay!)

skip-string-normalization

Ah well, fair enough. I don't plan to be pushy about this and I can easily live with this.

black bugs

Alrighty. The ones flagged here are fixed on black master so we're blocked on that release too then. :)

---

Folks, quickly flagging that we're blocked on isort and black releases, so let's defer further work and discussion until after they have new releases published. To that end, I'm gonna unsubscribe since... well, I'm sure that we'd have the opportunity to discuss before merging. :)

[BrownTruck]

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!

[pfmoore]

Can you elaborate on what the acceptance criteria are here

Sure. To repeat, I do not intend to unilaterally block this, I'm just stating my opinion, and noting that I'll leave it to someone else to decide to merge it.

My view is that using black should be a decision to "let the tool care about style choices". Style decisions are always contentious, and defined linter rules that we apply via CI is how we've handled this in the past. Moving to black, for me, means saying that our new style choices are simply described as "what black does", with the added benefit of having a tool that does the reformatting for us.

If we start picking and choosing which parts of the style black applies we're willing to accept, we're right back into the area of fruitless style debates, and I see no value in that, We also lose the benefit of having a tool that applies our style choices.

I personally:

1. Dislike intensely the sort of string concatenation within a line that it's been noted here that black can generate (and Merge implicitly concatenated string literals that fit on one line psf/black#26 suggests I'm not the only one).
2. Will put up with it on the basis of "we apply black and we're done".
3. Get very frustrated fixing code when the linter complains.
4. Would find applying black and still having to fix linter complaints intensely frustrating.

My preferences (in order would be):

1. Use black after it's fixed to avoid string concatenation within one line.
2. Use black, and not have linter rules that reject syntax that black applies.
3. Do nothing - don't use black at all.
4. Use black and have linter rules to force the occasional manual fix-up afterwards.

But I will accept any majority consensus regardless of my preference. My strongest dislike is reserved for going through even more rounds of debates over what style rules we like (I'm already accepting a number of choices made by black that I don't like - this is simply the one I struggle most to live with).

Objectively, I can see the benefit to contributors of "just run black on your code" as a style rule over "run the style checks (or let CI do it for you) and fix any reported issues". I consider "run black and then double check with the linters/CI in case you need to correct what black did" as distinctly worse than "just run black" and only a little better than what we currently have.

I'm aware that I'm repeating myself here. I don't really want to - it's prolonging a debate that I find frustrating and pointless. But I've actually yet to see anyone from @pypa/pip-committers give an unequivocal +1 to using black, so I'm unclear what's going on here. Are we simply going to drift into using black because there's a PR, and someone ends up hitting the merge button just to draw the whole issue to a conclusion?

[gaborbernat]

I don't have commit bit here, but if anyone wants to hear I'd be happier to contribute in a post black world, down the line when I finish virtualenv + tox rewrite 👍

[chrahunt]

To repeat, I do not intend to unilaterally block this, I'm just stating my opinion, and noting that I'll leave it to someone else to decide to merge it.

Thank you, and sorry to make you repeat it. I think having restated helps remove any doubt, especially since this can change over several months as the situation develops.

If we start picking and choosing which parts of the style black applies we're willing to accept, we're right back into the area of fruitless style debates, and I see no value in that, We also lose the benefit of having a tool that applies our style choices.

I think we could take a reasonable approach to not go against any intentional style choices made by black, but that doesn't need to be defined here. I'm OK to accept the output as-is and we can introduce additional linters (or not) separately.

Objectively, I can see the benefit to contributors of "just run black on your code" as a style rule over "run the style checks (or let CI do it for you) and fix any reported issues".

I would never suggest this - I would say "run tox -e lint" on your code, because that ensures that all the checks get run with the correct versions of all the tools.

That said, it was raised on #5425 that this wouldn't actually work on all platforms. To alleviate that I have submitted #7520 to fix the existing issues and ensure that these commands will continue to work going forward on Windows and macOS.

So the advice we would give for any contributors wanting to test locally will be:

1. get tox
2. run tox -e lint to check all style, types, etc
3. run tox -e pyXY -- -n auto to run tests

[chrahunt]

But I've actually yet to see anyone from @pypa/pip-committers give an unequivocal +1 to using black, so I'm unclear what's going on here.

(as someone in @pypa/pip-committers) I vote +1 to using black. I think it provides very clear benefits including and above the ones I stated in #7084 (comment).

[pfmoore]

So the advice we would give for any contributors wanting to test locally will be:

1. get tox
2. run tox -e lint to check all style, types, etc
3. run tox -e pyXY -- -n auto to run tests

I'm atypical, but as someone who develops on Windows, and often on an unusually (very!) slow machine, this can be way more time than I have available. Leaving tests and linting to CI is normal for me.

(as someone in @pypa/pip-committers) I vote +1 to using black.

Thanks. To be completely clear, are you in favour of, or opposed to, having a lint check that rejects the implicit string concatenation on a single line that black can sometimes introduce?

[pradyunsg]

Thanks for elaborating @pfmoore!

I'm aware that I'm repeating myself here. I don't really want to - it's prolonging a debate that I find frustrating and pointless.

I understand and empathize. I also have fairly strong disagreements with certain style choices that black makes. IMO we all do a fairly excellent job of not wasting our time on style discussions in PRs as well.

That said, the tool does produce a consistent output, which is very convenient when working on the codebase, and it enables us to not have code-formatting discussions with contributors. I recently started using black + pre-commit + nox workflow on my personal projects (and vendoring + more upcoming), and I'm pretty happy with it.

I'm a +1 on formatting code with black in pip's codebase.

---

As @chrahunt points out, we can do these improvements incrementally, so we can start with 2, and then move forward however we want (1, 4, or even 3). :)

[pfmoore]

Thanks both for your comments. As we're still blocked on the isort release, I guess we go back to letting this lie until that's sorted out. At which point, if psf/black#26 remains unresolved, I suggest that unless someone else comes up with a strong objection in the meantime, we go ahead without a style check that rejects the one-line string concatenation¹, and see how we get on from there.

¹ BTW, I have no objection to any instances being fixed as part of the "use black" PR, I just don't want to enforce it

[pradyunsg]

Sounds about right to me. :)

[chrahunt]

To be completely clear, are you in favour of, or opposed to, having a lint check that rejects the implicit string concatenation on a single line that black can sometimes introduce?

In favor.

[xavfernandez]

For the record, I'm also in favor of adopting black (and "having a lint check that rejects the implicit string concatenation on a single line").
But I agree with Paul that this could be unpleasant and confusing for contributors:
a tox -e lint-fix target launching isort, black and other potential "fixers" would certainly be helpful.

[sbidoul]

FWIW as a data point, when we introduced black and pre-commit in the Odoo Community Association, it went very well and actually made the contribution process simpler.

For the few contributors who are confused by lint errors in CI, instructing them to run pre-commit run -a locally is sufficient and easily understood. It gives a mix of automatic fixes and errors that are usually very easy understand and fix manually.

So for me the sooner we can apply black on the pip code base, the better.

[ssbarnea]

Can someone rebase this please? The sooner we see it in the better.

[hugovk]

We're still pending an isort release: #7084 (comment).

Once released, I'll update this PR.

[pradyunsg]

I posted a polite nudge for the isort dev, looks like they made some commits a few hours ago so I'm hoping we'd get a response from them soon. ^.^

Worst case, if we don't have a response in a month, let's pin our isort pre-commit hook to a commit hash containing the fix that we care about. (I think we should be able to do that)

[johnthagen]

FYI, looks like isort 5.0.0 is releasing tomorrow: PyCQA/isort@2ea917e

[pradyunsg]

And, it's been released. :)

https://pypi.org/project/isort/5.0.2

[pradyunsg]

And, psf/black#1132 makes black's handling of strings so much better.

@hugovk Would you be willing to take this forward still, or would it be OK if I pick this up?

I'd like us to close this PR and open a new one now, since the CI checks have been updated, and it'll be much nicer to have all the checks be up-to-date in a new PR rather than to have a mix of old and new checks in the PR view.

[hugovk]

@pradyunsg Happy either way! If you'd like to, go for it!

isort is now at 5.0.3, might be worth waiting a bit for things to settle (and good to see the quick updates and releases).

Yes, good idea to close this and start afresh.

[deveshks]

Hi @pradyunsg

I would also like to help out in the effort of introducing black to the codebase. I would assume that we would want to start by updating a few files in a module at a time (instead of all files in one go), to keep the PR's short and reduce disruption in other open PRs.

[pradyunsg]

Awesome. I've gone ahead and filed a tracking issue for doing this and self-assigned it, since I'd like to take this up myself (after the 20.2 release).

@deveshks I think we're going to be updating the entire codebase in one big sweep commit, mostly since the changes are automated and, thus, much easier to audit (run black on the previous commit) so it's not necessary to break it up. And, we're going to be almost-equally disruptive regardless of how staggered we make the update, so doing it in one-big-sweep gives us a clean cut which is easier to communicate around. :)

[johnthagen]

For those tracking, new issue is #8543