[WIP] Format code with black

[dstufft]

Black is an automatic code formatter that has opinions. This is similar to gofmt in the golang community. I would like to switch pip over to automatically formatting code using black (and enforcing that check in the CI).

This has a number of benefits, the primary one being that it mostly eliminates the need for human beings to worry about formatting at all. There are still a couple of edge cases where a human might have to worry, but generally it just works.

This also means that for contributors sending PRs, they don't have to worry about wether a human is going to "nitpick" their PR formatting or not. A machine will do it and if the machine tells them they are wrong, all they have to do is run tox -e format and it will fix it for them.

This is a WIP because I'm hitting an issue with combining black and isort (which is supposed to work, but not sure if It's a black bug or a bug with my settings) and because it currently won't run on Windows. If there are no objections to this, then I'll work with @ambv to get an --ignore option added to black so that we can drop the Unix specific shenanigans to ignore the pip/_vendor directory.

Fundamentally though, the idea here is to reduce manual effort and let us focus on important parts of the code, rather than having to sit there and tweak and tweak code formatting to please some tool-- the tool will just do it for us.

[pradyunsg]

I like this. I was going to suggest the same but didn't get the bandwidth to start this discussion.

[pfmoore]

Can I confirm - is the idea that PRs can be formatted any old way, and an auto-format step will run before merging. Or will contributors be required to install and run black before creating PRs? (I'm assuming the former, but want to be clear).

[dstufft]

@pfmoore the latter, except that they don't have to manually install black, since it's a tox command, as long as they have tox installed, they just run tox -e format and it installs black and formats the code for them.

Unfortunately Github doesn't provide a way to have an autoformat step before the merge.

Of course there are things like commit hooks, and editor integrations that means for folks who want that, their editor or git itself can automatically run black for them, so they never have to manually invoke anything.

[pfmoore]

:-(

So what, our lint checks will fail and the users will need to know to (install tox and) run tox - format or install black and run whatever the relevant command is? Honestly, I'm not keen on that. On a purely personal note, it's a mild PITA for my personal workflow (which involves working on multiple machines and not having a "proper" development environment most of the time), and I worry it increases the barrier for people offering quick fixes.

But if it's what others prefer, I'll live.

[pradyunsg]

@dstufft Just running "black" on the CLI will also work, correct?

[dstufft]

@pfmoore I mean, the current situation is if the lint checks fail you're stuck manually tweaking it in the hopes that your new tweak fixes it. You can of course continue to do that! The new linter is stricter so trying to manually tweak things is in a way harder, but it's also far more consistent, so it's also in a way easier.

So having to do something on your local dev when linting fails is already the status quo, this is just provides a tool you can run that will automatically fix it for you.

And yes, like @pradyunsg said, you can just run black on the CLI too, though you'll need to pass a flag to ignore the src/pip/_vendor directory (once we have that flag implemented).

[dstufft]

@pfmoore Would it be helpful if the linter showed you what it expected? I don't know how @ambv would feel about adding a --diff flag to go along with the --check flag, but we could certainly ask for one.

[ambv]

@dstufft: you can combine --check and --diff already on the command-line.

[dstufft]

Oh, I totally missed that option! Well there you go, so the lint output can tell you exactly what is wrong :)

[pradyunsg]

you'll need to pass a flag to ignore the src/pip/_vendor directory (once we have that flag implemented).

:(

@ambv Would you consider adding configuration for Black (in pyproject.toml maybe) -- would be nice for extremely project-specific things like this?

[pfmoore]

@dstufft Don't worry about catering for my (unusual) workflow. I'm perfectly OK with falling in line if that's what people want.

I would suggest that lint errors display the command needed to fix them. For me, it'd stop me forgetting (is it tox black or tox format, or what flags do I need to add to run black manually?[1]) And it'd stop unsuspecting contributors trying to manually fix lint errors without even knowing there was a one-liner to do it for them.

[1] Personally, I'll probably run black manually - the extra time for tox to build an environment is non-trivial for me, so I wouldn't typically use that.

[RonnyPfannschmidt]

@dstufft please consider combining this with pre_commit

• see https://github.com/pypa/setuptools_scm/blob/master/.pre-commit-config.yaml for a example config and https://pre-commit.com/

[ambv]

@pradyunsg Black doesn't implement its own configuration files. Any CI integration requires you to specify command-line options anyway. tox.ini is a good choice.

I'm not saying we will never grow a configuration file and pyproject.toml is a likely candidate if that ever happens. For now though, I think we can live without that.

[pfmoore]

@dstufft My main concern is that it remains possible to create PRs for pip with a minimal installation. I'm OK with "you need to create a virtualenv and install black and run it on the pip directory". I'm less happy with "you need to remember these following arguments to black". I'm definitely unhappy with "you need tox", not least because, as currently defined, the tox format environment is Unix specific and won't work on Windows.

BTW, where can we have the argument about the specific style choices made by black? To give one example, I'm not keen on its "all strings use double quotes" ruling. If concerns about the style rules were my only problem with this PR, I wouldn't complain - but in conjunction with my other concerns, having to learn to live with style rules I don't like is yet another problem with this proposal for me.

@RonnyPfannschmidt Can you clarify (for someone who has never used it) what the implications are on pip developers' workflow from using pre_commit? Or not - it's definitely off-topic for this PR, but if @dstufft does take your suggestion, let's debate that point there.

As a general point, I'd like to see some care taken over changes that alter "what tools and workflow you need if you want to work on pip". Maybe we should add a "how to hack on pip" section to the docs, that clarifies the minimum environment we expect people to have.

[pfmoore]

Actually, regarding the point about tox.ini, would it be possible to split this PR into (1) the changes to enable black, and (2) the cosmetic changes black makes? It would be much easier to review the process changes if they weren't lost in the style changes...

[dstufft]

I can do that, but you can also just look at the first commit. It’s all contained there. FWIW the unix specific hack is temporary and would be fixed before this landed. I just wasn’t going to ask @ambv to implement —ignore for our use case until we knew if we were going to do it or not.

…

Sent from my iPhone

On May 18, 2018, at 5:34 PM, Paul Moore ***@***.***> wrote: Actually, regarding the point about tox.ini, would it be possible to split this PR into (1) the changes to enable black, and (2) the cosmetic changes black makes? It would be much easier to review the process changes if they weren't lost in the style changes... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[pfmoore]

@dstufft OK, I didn't think of looking at the individual commits. That's fine.

Happy if the tox command gets fixed to be multi-platform, but I'd still not want to use tox in practice, so I assume the canonical invocation would be (from the pip top level dir) black --ignore src/pip/_vendor src. Is that the intention?

I bet someone will forget the exclusion and mess up a PR at some point :-) I'd much rather that exclusion was in a config file within the repo somewhere. but #5425 (comment) implies that's not happening in the immediate future :-(

[dstufft]

@pfmoore Yea, it'd look something like that, could possible even make it just black --ignore src/pip/_vendor and omit the src/ directory.

If @ambv accepts a config file for the handful of config options black actually has (currently just line length, we'd be proposing an additional ignore optionthen we could get that down to justblack``, which I think is pretty hard to beat in terms of UX. It's all pure Python as well afaik, so super easy to actually install.

[dstufft]

WRT the to pre-commit hook ting, I've never used it, and I'd probably want to include that in a separate PR if we end up doing it. Just because I don't think it's required for this PR, and since this touches a lot of code, it's easier to keep the actual non-mechanical changes as small as possible.

[pradyunsg]

@dstufft I've merged a PR on master and that's caused merge conflicts on this PR. :/

They'll be easy to "resolve" though I think -- rebase, cherry pick and run black. (correct me if I'm wrong)

[dstufft]

Oh yea, it's trivial to fix it. Not going to bother until black has the features we need.

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

[xavfernandez]

I personnally don't really like the output formatting but I think not having to worry about formatting at all is worth it :)
So globally +0 on the change 😛

[dstufft]

I've reviewed as much as I have time for right now (about 1/4 of the changes). There's a lot of style choices I don't like, and a few of what look to me like bugs in black. Not sure how we feed those back, but I do think that we should get the bugs fixed before we do anything to implement this.

First of all, thanks for taking the time out to take a look at this. I really appreciate it no matter if we merge it or not! For feeding those back, I know that @ambv is watching this issue, so will likely see the issues raised and can make calls on whether things are exposed issues that need fixed, or if they're just black opinions and that's how it is in black.

One other point - black's use of a non-standard 88 character line length. Is that going to mean that users will have to configure their editor differently to work with pip? I can imagine us getting a lot of churn in PRs because black wants to put something on one line where users' editors say that line's too long. I'm inclined to say that we should configure black to use a PEP 8 standard line length. I really don't like deliberately not following PEP 8. My understanding of all the other black rules is that they conform to PEP 8, they are just stricter - but this one is a direct violation of PEP 8.

Yea. I can tune this back to the PEP8 standard line length. I left it at the default because I personally don't care one way or the other, so the default seemed perfectly fine to me-- but I can easily switch it. I'm not going to push another review diff that does that, because I don't think it'll be helpful in reviewing the output. It would just change some of the cases where black put things onto a single line into cases where black puts things on multiple lines.

[dstufft]

I'm withdrawing this PR, for more details on rationale, see #5425 (comment). Thanks everyone for taking the time to look through this!

[pfmoore]

Thanks for taking the time to work through all this. For the record, I'm not against automatic code formatting as such. If we could find a tool that used a style that we were all happy with, I'd be happy to reconsider.

[ambv]

Thanks for the thorough review! Given that this is already closed I didn't come back to respond on all the inline comments. Some are easy to work around, a few are bugs, some I disagree with. That's beside the point now.

In general I feel like this is one of the cases where developers like @pfmoore are attached to full control over code formatting within their projects. That's an honest position to have and I can understand it, given how sweeping Black's formatting is.

Black is a young project and so is its documentation. If you have suggestions how to make it better, I'd be happy to do so. Currently it's mostly designed to be read from top to bottom, in which case everything falls into place.

[pfmoore]

@ambv Thanks for the reply. There's certainly an extent to which I would say that I want control over how I format the code I write (and conversely, I want to let other contributors format their contributions how they feel works best, within the overall project guidelines/rules). There's also a large extent to which my position is simply that I don't like the choices black makes. That's fine, of course - style guides always generate controversy.

I think my main suggestion for black is twofold.

1. (I suspect you won't want to take this option - I'm putting it here mainly for contrast). Make black a lot more configurable. The idea of a formatter that enforces a project's style guide is a great one, and if black could be tuned to fit project standards (with a set of defaults that act as "if you don't care, or just want to follow an existing set of choices, use these") it could fit that role well.
2. Present black as a set of programming standards, with the incidental benefit of having an auto-formatter. This is a matter of presentation and perception, and the community in general has as much to play in this as the black project does, but by promoting black as a tool, people buy into the idea of tool-based formatting, and the style choices get imposed almost incidentally.

To explain point (2) a bit more, this PR was entitled "Format code with black". That sounds pretty innocent - basically, let's use a tool that goes a step or two further than our existing style checks, that'll make life easier. But in actual fact, the main point of the PR was "Introduce new coding style standards to the project, which are a lot stricter than the current ones" (the additional tooling is a useful process improvement that eases the effort needed to adhere to the new stricter rules). With a title like that, I suspect the proposal would have been much more controversial.

Furthermore, as time goes on and more and more projects pick up on black, I imagine it will get less and less reasonable to suggest changes to the rules, because they will impact so much code. I don't know what the go experience is like here - has there been much change in the rules enforced by gofmt? PEP 8, on the other hand, is fairly regularly updated with new suggestions and clarifications.

I don't think this is any sort of "conspiracy", it's just the way the tool is being picked up by projects. Although I think the black docs can affect the perception - in fact they can't help but do so.

Regardless of anything else, this has been an interesting discussion, and it's made me think a lot more closely about code style and what I consider to be "readable" code. So I've definitely benefited.

[gaborbernat]

@pfmoore can you elaborate why you are so much against making tox a requirement to run tests/develop locally?

As for this whole thread, I think you miss interpret the power of black. It is not that it forces horrible code style on you. It's that it relinquishes arguments about code style in future. All too often new people are discouraged to contribute to a project because creating a PR means you have to argue about style with the maintainers; which I think is not healthy for anyone. It's a lot more important to have a consistent and automated style; rather than "readable" by someone's definition. Let's focus our energy on creating application logic, not arguing about style. I would say introducing black would help a lot to make the code base more readable by not having diverging styles, and stopping style arguments once and forever on this project, and relegating to the blacks issue tracker.

[dstufft]

Furthermore, as time goes on and more and more projects pick up on black, I imagine it will get less and less reasonable to suggest changes to the rules, because they will impact so much code. I don't know what the go experience is like here - has there been much change in the rules enforced by gofmt? PEP 8, on the other hand, is fairly regularly updated with new suggestions and clarifications.

I believe that gofmt has made some changes since 1.0, not a lot and it's fairly rare. With a tool like gofmt, black, etc changing the formatting is a bit funny. You're incentivized against doing it because it's a bunch of churn in every project that uses it. However, when you actually do it, it's not really that hard to deal with, because by definition all you have to do is run black and you're back in a "good" state.

[sersorrel]

@gaborbernat:

All too often new people are discouraged to contribute to a project because creating a PR means you have to argue about style with the maintainers

As someone who contributes to various random projects, a much bigger factor in whether I try to get my changes upstream is how much effort that requires.

• If I can just submit a PR straight away (maybe with something like pipenv install -e . && pytest first), then it's really likely that will. Everyone wins :)
• On the other side of things, there's a library I used briefly that required contributors to spin up an entire Vagrant box to run the tests! There's no way I was going to go to that much effort to essentially fix a typo, so I didn't. :(

In the first scenario, even if I decide to abandon the PR for some reason, my code is still there, and someone else can fix the problems with it and commit it (if that's worth doing) - still a win. In the second scenario, nobody but me ever sees my code, so there's no way for anyone else to benefit from it.

(Also, I don't think I have ever abandoned a PR due to being asked to make changes - is this a common thing? Are people actually being scared away before they even start contributing by the prospect that someone might point out a potential improvement to their code? Not a rhetorical question - if that's true, I would like to know.)

[gaborbernat]

@anowlcalledjosh

If I can just submit a PR straight away (maybe with something like pipenv install -e . && pytest first), then it's really likely that will. Everyone wins :)

Personally, I prefer taking my approach even easier by just reducing to tox -e test, format. Yeah, I know pipenv is nice but is not everyone's favourite tool. Also, complex projects have multiple types of tests that may require passing some flags to pytest; so in practice is not always as simple. And then follows coverage; I generally want to create a PR that does not decrease coverage. I find having simple tox targets for those is a lot simple than needing to remember complex incantations.

On the other side of things, there's a library I used briefly that required contributors to spin up an entire Vagrant box to run the tests! There's no way I was going to go to that much effort to essentially fix a typo, so I didn't. :(

Vagrants box are overkill, I agree. Here I asked @pfmoore only to name why he does not like tox; given it's just has a single dependency (Python). Which should be already available for the user.

(Also, I don't think I have ever abandoned a PR due to being asked to make changes - is this a common thing? Are people actually being scared away before they even start contributing by the prospect that someone might point out a potential improvement to their code? Not a rhetorical question - if that's true, I would like to know.)

In my experience, this is the most major factor in delaying acceptance of PRs. Yeah people tend to do it, but that's mostly because they've invested time in the business logic. But makes a PR which could be accepted in 1 day to be needed to work on in 2 days, just to transform the code to the code bases style.

Here's two random example:

• First review batch of this Github Enterprise Support readthedocs/readthedocs.org#3853 (comment)
• Or change requests here: option to merge stubs into source trees python/mypy#5139 (review)

In both cases, I will or did the style changes because I want the business logic to be accepted. But I really could use my time better than making stylistic changes. It adds an overhead for me to identify and alter all of them. And then for someone to review it again that I did not miss any of those. Would it not be better to focus on improving on the architecture and logic of tools; rather than the style how you express it (for both contributors and maintainers)?

TL.DR. All our time is precious, we have a limited amount to work on open source, so let's focus on adding value as much as possible; we should automate as much as possible about how we express it. I'm not saying black is the one and only and the true holy grail format. It's miles better than not having an automatic formatter though.

[pfmoore]

Coming back to this basically because @pradyunsg mentioned it in #7078, my feelings towards black have softened somewhat. I've needed to use it in a couple of other projects, and have come to appreciate the "no need to even think about code style" approach.

As the person arguing most strongly against black here, I feel that I should go on record saying my feelings have changed.

I'm still not sure I'd claim to like all the style choices it makes, but if a similar PR came up now, I'd probably shrug my shoulders and say let's go for it. (I'd still want the formatting command to be just black, without needing specific command line options, though - if that doesn't work for us, aren't we still arguing style choices which was the whole thing we wanted to avoid?).

[pradyunsg]

@pfmoore Would tox -e format work for you?

[pfmoore]

Partly. But someone is still going to just run black directly, even if only by accident, as that's the normal approach (that "someone" could easily be me). So while tox -e format is sufficiently convenient in one sense, it still has problems IMO.

I didn't think there were any significant style options available for black. What precisely would the difference be between tox -e format and plain black? (But I'm honestly not clear on the reason we'd say "use black" and then not go with the standard style - as I said I don't necessarily like all of the style choices, but IMO there's no point agreeing to use black and yet still arguing about style choices...)

[gaborbernat]

All potential back configuration can be added to the pyproject.toml so the end user would only invoke black. Import ordering is something we would want via isort?

[hugovk]

Yep, isort is already part of tox.

Remember to update the isort and flake8 config to be compatible with Black. See examples under https://github.com/psf/black#how-black-wraps-lines and https://github.com/psf/black#line-length.

[pradyunsg]

Import ordering is something we would want via isort?

We already do this. :)

Okay then, I think we'll discuss further if/when there's a PR for doing this now.

[pradyunsg]

someone is still going to just run black directly, even if only by accident, as that's the normal approach (that "someone" could easily be me)

Fair. As @gaborbernat points out, it should be fine as long as we have all the configuration in pyproject.toml.

[hugovk]

Please see PR #7084.