I’ve done a bit of testing with both tools, and I’...
# general
f
I’ve done a bit of testing with both tools, and I’d like to explain the pros and cons of both approaches and why I think Black is a better option. So the main difference I was aware of from reading both projects descriptions was that yapf would allow us to configure it so that we could minimise the changes from our de-facto style. Most importantly, we use 2 spaces for indentation and I would be quite happy if I didn’t have to convince developers to change their style for such a potentially contentious detail. So my mindset was: everything else being equal, let’s make yapf work and pick it up. So I went ahead and tried both tools on the pants code base. Here is what I found: • yapf fails to format our code when running it on the entire pants codebase. I throws an exception and doesn’t indicate which file was troublesome to parse. • When black fails to parse a file, it flags it up and keeps going. If you run it again, you can see the list of files that it failed parsing. • When running both on the same directory (contrib, after hunting for a directory where yapf wouldn’t throw an exception in the middle of its execution), I get these timings: • Black: 3.5 seconds • yapf: 12 seconds • If I rerun on the same directory, Black does some caching, so I get • Black: 0.4 seconds • yapf: 12 seconds One of the reasons Black performs better is that it leverages all my machine’s cores where yapf doesn’t bother to. In all the files in pants, exactly one gives an issue to Black: unicode/compilation_failure/main.py; because Black trips up on its invalid syntax. This can easily be added to an ignore list in Black’s config file. Based on these observations, I am pretty convinced that Black is a more robust tool and will give us less grief using it, while giving us the benefits of consistent and automatic formatting. Now for the elephant in the room: Black’s opinions. Personally, I am quite happy to delegate stylistic decisions to a body of developers (the Black developers) who thought of it very hard and came up with something they would consider “sensible”. I truly believe automation and consistency both trump any sensitivity I could have about the specifics of how to layout code. It is pretty easy to get used to a certain style if it is used consistently throughout a codebase. Specifically, this means I would be fine with switching to 4 spaces since Black doesn’t allow 2. I think the code produced by Black is quite readable, after browsing through pants post-formatting. Black does allow the user to configure the line length, and I would advocate for either 88 (which is their default) or 100 (which seems to be our de-facto standard). I would probably lean a tiny bit towards 100 lines because I think that may be what a majority of pants devs prefer, although I’m completely fine either way. I have two questions I would like everyone who cares to answer: • Would you be OK to switch to Black? If not, why and what would be your preferred course of action? (yapf, nothing, …?) • If yes, would you be OK with 100 for lines length? If not, why and what length would you prefer? Looking forward to hearing your thoughts 😄
❤️ 7
h
100% on board. Thank you for proposing this. In a few past reviews, we’ve all had to waste time on a formatting issue that could have been fixed automatically by Black Black is also becoming increasingly standard in the python community. Pytest, Django, and even CPython’s dataclasses module use it
❤️ 2
s
+1 - I am an early adopter of yapf. New projects should use black instead.
👍 3
b
No strong opinion from me, I've used black on side projects and have enjoyed the experience. I guess that would be a +0.5 for black from me.
👍 2
a
I haven’t read the whole message (let me know if it’s important that I do), but my answers to both of your questions are “YES, YES PLEASE, THANK YOU” 🙂
😀 3
f
b
I like black, but it’s also configurable. I’d say make sure it honors the black configuration files and then it’s fully integrated.
2
👍 2
because monitors are bigger these days
f
This is the entire config that I have in mind at the moment (in pyproject.toml)
Copy code
[tool.black]
line-length = 100
exclude = '''
/(
  | \.git
  | \.mypy_cache
  | dist
  | \.pants\.d
  | virtualenvs
  # This file intentionally contains invalid syntax
  # It trips black up.
  | compilation_failure
)/
'''
b
can we make it a separate file that can be changed? then it’s just “drop in your favorite black config if you don’t like ours”
f
Ah sorry! I meant: in pyproject.toml for pants. Of course, every user would use their own pyproject.toml in their repo and that would apply
So pants would dumbly invoke black, and black would pick up the settings from this config file in any repo pants is run on
b
but as a user, where do I go to configure it?
f
in pyproject.toml in the root of your repo
under the [tool.black] entry
b
I think that’d work
👍 1
s
@fancy-motherboard-24956 My answer to both the questions - yes!! Looking forward to saving some time formatting before posting any diffs
❤️ 2
f
@bitter-waiter-91252, I may have been unclear: the pyproject.toml file is the default location where Black looks for its settings. See https://github.com/psf/black#pyprojecttoml I’m not suggesting pants overrides this. pants would simply run Black and the settings would be read as if you were running black manually. So in a way, I’m just repeating your first comment 🙂
b
gotcha - for some reason we started with a different black config file
👍 2
the perils of using tox
👍 1
e
I'm on board for black. My only opinion is we should definitely keep the line length 100 if we are switching to 4 space indents.
💯 2
👍 2
w
Black sounds better from what’s described. Thanks, Pierre!
❤️ 2
w
@fancy-motherboard-24956: to be clear: the right way to integrate black would be to make a pants task for it in the
fmt
goal
👍 4
so with regard to where the config lives: it would live where pants was configured to look for it
f
Having pants decide where to pick up the config is something that can be done, probably without too much effort. I think I wouldn’t consider it a blocker to getting a first version out and dogfooding it, though.