Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format code with Black #7084

Closed
wants to merge 2 commits into from
Closed

Format code with Black #7084

wants to merge 2 commits into from

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Sep 26, 2019

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:

@hugovk
Copy link
Contributor Author

hugovk commented Sep 26, 2019

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
Copy link
Member

pfmoore commented Sep 26, 2019

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.

@@ -19,7 +19,7 @@ repos:
exclude: .patch

- repo: https://github.com/timothycrosley/isort
rev: 4.3.21
rev: 18ad293fc9d1852776afe35015a932b68d26fb14

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we wait for a. release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pradyunsg pradyunsg added state: blocked Can not be done until something else is done skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes labels Sep 26, 2019
@pradyunsg
Copy link
Member

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
Copy link
Member

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

@pfmoore
Copy link
Member

pfmoore commented Sep 26, 2019

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 pradyunsg removed the skip news Does not need a NEWS file entry (eg: trivial changes) label Sep 26, 2019
@pradyunsg
Copy link
Member

pradyunsg commented Sep 27, 2019

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
Copy link
Member

@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
Copy link
Member

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
Copy link

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!

@pypa-bot pypa-bot added the needs rebase or merge PR has conflicts with current master label Sep 27, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 27, 2019
@hugovk
Copy link
Contributor Author

hugovk commented Sep 27, 2019

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:

@pfmoore
Copy link
Member

pfmoore commented Sep 27, 2019

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
Copy link
Contributor Author

hugovk commented Sep 27, 2019

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

@pfmoore
Copy link
Member

pfmoore commented Sep 27, 2019

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
Copy link
Contributor Author

hugovk commented Sep 27, 2019

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

pfmoore commented Dec 28, 2019

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
Copy link

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

pfmoore commented Dec 29, 2019

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
Copy link
Member

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
Copy link
Member

pfmoore commented Dec 29, 2019

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 concatenation1, and see how we get on from there.

1 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 pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Dec 29, 2019
@pradyunsg
Copy link
Member

Sounds about right to me. :)

@pradyunsg pradyunsg removed the skip news Does not need a NEWS file entry (eg: trivial changes) label Dec 29, 2019
@chrahunt
Copy link
Member

chrahunt commented Dec 29, 2019

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
Copy link
Member

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
Copy link
Member

sbidoul commented Dec 31, 2019

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
Copy link
Contributor

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

@hugovk
Copy link
Contributor Author

hugovk commented Jan 24, 2020

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

Once released, I'll update this PR.

@pradyunsg
Copy link
Member

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
Copy link
Contributor

johnthagen commented Jul 3, 2020

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

@pradyunsg
Copy link
Member

And, it's been released. :)

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

@pradyunsg
Copy link
Member

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
Copy link
Contributor Author

hugovk commented Jul 5, 2020

@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.

@hugovk hugovk closed this Jul 5, 2020
@deveshks
Copy link
Contributor

deveshks commented Jul 5, 2020

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.

@hugovk hugovk deleted the format-with-black branch July 5, 2020 10:56
@pradyunsg
Copy link
Member

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
Copy link
Contributor

For those tracking, new issue is #8543

@pradyunsg pradyunsg added this to the The Blackening milestone Sep 23, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs rebase or merge PR has conflicts with current master state: blocked Can not be done until something else is done type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.