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 😄

On consideration, let me ping <!channel> to be sure everyone gets a chance to be notified of this discussion, so we don’t miss any point of view ^^

The PR to add initial Black support to pants (as a v2 rule) is up. Getting there 😄 https://github.com/pantsbuild/pants/pull/8350

welcome @dry-musician-84585!

is there any reason why https://github.com/pantsbuild/pants/issues/7726 still needs to be open?

Is there a particular reason the SetupPy task raises an error if there are no tasks? All other tasks seem to silently complete when they do not have any tasks to execute. https://github.com/pantsbuild/pants/blob/44d4ab5e272094314bff498ef14013a53df3a814/src/python/pants/backend/python/tasks/setup_py.py#L667

I am building a plugin that will consume the

python_dists

if present as a dependency. This is causing my builds to fail when no

python_dist

dependency is present

Can anyone point me at where

RunTracker

is actually initialised? I’m trying to access

RunTracker.global_instance()

in places where I expect to be able to, and it’s `None`…

is the convention we're trying to adhere to for test files

test_xxx.py

or

xxx_test.py

?

Has anyone worked on integrating pants with arcanist recently? I have found an older thread on the ML from 2016 but nothing more .. https://groups.google.com/forum/#!searchin/pants-devel/arc%7Csort:date/pants-devel/ZG4GjotZ0bs/I1J_inC4AQAJ

How opposed would we be to make

CommandRunner::run

take a

&mut self

instead of an

&self

? The reason I ask is: - In the context of https://github.com/pantsbuild/pants/issues/8311, we want to keep a mutable map of live nailgun processes somewhere

Core

-scoped. - We will probably need to either heavily alter

local::CommandRunner

to add nailgun, or create a new

NailgunCommandRunner

that wraps it. Currently, neither local nor a custom command runner could hold the map of nailguns, because

run

takes an immutable reference. The solutions I can think of right now: - The map can go in

Core

, and we pass it to the respective CommandRunners, changing the signature of

CommandRunner.run

to be

run(&self, epr: MEPR, WorkunitStore, &mut NailgunPool)

. - The map can go into a

CommandRunner

, and we could handle connection separate to

run

, but then we are special-casing nailgun in the graph. - The map can go into a

CommandRunner

, and we could handle the connections at

run

time, and then we’d need to modify the signature of

run

Can someone help me pin a specific version of a transitive dependency of pytest in my pants.ini file? Or if that's even the right place to do that? For background, I am using pytest 3.4.2 which has a transitive dependency on attrs. Yesterday, attrs pushed a new version that uses deprecated functionality that's breaking my builds using pytest. I've been trying to pin attrs==19.1.0 in my pants.ini file, but no clue how to. Thanks!

Where are you setting your pytest version?

Under

[pytest]

in pants.ini?

Question for engineers who work at Twitter, does twitter have code search engine internally?

California now is sleeping. I forgot.

i generally wouldn't expect a response earlier than a day from slack :) pants isn't a super huge community yet

twitter has a code search tool but it's nothing super special, just trigram search i think

Awesome.

how do I register a test-specific class as a goal within a test?

hm?

will answer in #engine

https://pantsbuild.slack.com/archives/C046T6T9U/p1569584507077100 So after the v2 target was merged yesterday, the Black reformatting PR is up https://github.com/pantsbuild/pants/pull/8391 Not quite the end of the road, yet. I wouldn’t be surprised if CI took some cajoling and I plan to publish the git hook + git blame fix in a follow-up PR, but excited to get that one in 😄

Seeing this in my branch CI now (thanks to Travis idiotically merging master into branches before running CI):

[ERROR] Invalid scope [black] in /home/travis/build/pantsbuild/pants/pants.ini

Re the reformatting PR, it is truly great! However I am concerned about merging such a big disruptive PR, causing merge conflicts for every outstanding change, while we're in a CI crisis and having trouble getting anything merged.

It might be better to do this in small chunks?

hello guys, maybe a stupid a question, but I'm trying to install a third-party package from Github (with some Cuda/c++ that need to be built), is there a way to add it to the BUILD file of 3rdparty? I've seen the example of

custom_tensorflow_op

, but that is just for wrapping and loading the .so file (the repo I want to add : https://github.com/facebookresearch/maskrcnn-benchmark)

thanks 🙂

not at all a stupid question!!! that backend is still quite new -- but i'd love to help you make this work out!