test: Add .style.yapf #15479

This can optionally be used to format any added code before submitting a pull. I use this heavily and wouldn't want to hold it back from others, now that yapf is referred to in https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md#format-python-diffs-with-yapf-diffpy

Just to mention the obvious: No linter is added, using this is not enforced in any way. Same as #4498

@MarcoFalke Out of curiosity: have you tried black? Some major Python open source projects switched over to black during 2018. Looks like it has a good chance of becoming the de facto standard -- a gofmt for the Python world :-)

Concept ACK

black does not work on diffs

I'm vaguely against this if it encourages people to submit style changes that aren't part of the style guide. I find it irritating when I'm reviewing a PR and a bunch of the diffs are "I've inserted this extra whitespace because my editor told me to" (eg #15419 (comment) and #14179#pullrequestreview-153839345 and several others). PRs should not mix functional changes with code style changes, which is what this would encourage.

I am using this tool for years now, and this is the first time that I had someone complain about how I use it. Though, I am happy to split the changes up into several commits in the future, if that is what you prefer.

@MarcoFalke Is this config meant to mirror PEP-8?

If it is meant to mirror PEP-8; why the deviations from yapf3 --style pep8 --style-help?

If it is not meant to mirror PEP-8; why? Why should we buy --style MarkoFalke instead of the market leading brand --style pep8? :-)

Diff between --style pep8 and --style MarkoFalke:

--- yapf3-pep8.txt      2019-02-26 23:33:41.147520979 +0100
+++ yapf3-marko.txt     2019-02-26 23:33:56.339389554 +0100
-allow_split_before_dict_value=True
+blank_lines_around_top_level_definition=2
-coalesce_brackets=False
+coalesce_brackets=True
-column_limit=79
+column_limit=160
-each_dict_entry_on_separate_line=True
+each_dict_entry_on_separate_line=False
-no_spaces_around_selected_binary_operators=set()
-split_before_bitwise_operator=True
+split_before_bitwise_operator=False
-split_before_expression_after_opening_paren=False
-split_before_logical_operator=True
+split_before_logical_operator=False
-split_complex_comprehension=False
-split_penalty_after_opening_bracket=30
+split_penalty_after_opening_bracket=0
-split_penalty_comprehension=80
-split_penalty_excess_character=4500
+split_penalty_excess_character=3500

Changed to the yapf pep8 style:

$ yapf --version 
yapf 0.24.0
$ yapf --style-help --style=pep8 > .style.yapf

The current config in this PR seems equivalent to the default style of yapf 0.26.0 (--style pep8) with the following exceptions:

allow_split_before_default_or_named_assigns=True
arithmetic_precedence_indication=False
indent_blank_lines=False

I assume these three were added in 0.25 or 0.26. @MarcoFalke I guess I'm missing something but what benefits do adding a custom config file .style.yapf bring if we're using the defaults? Wouldn't just running yapf without any config file yield the same results? :-)

No, it would search for your personal style in your home dir or other parent directories

@MarcoFalke Thanks for clarifying. That wasn't clear from reading the PR description.

I'm vaguely annoyed that this was merged by the PR contributor without any ACKs and my outstanding concept NACK. I don't think Python style matters that much but I think as a policy PRs from maintainers should have the same burden of review as everyone else's PRs.

I don't see how this could be controversial. How is this different from .python-version? No one forces you to use that python version specified there.

Similarly our guidelines recommend to use pep8, which is what this yapf specified: #15479 (comment). No one forces you to use pep8 or the style specified here.

FWIW I agree with @jnewbery here.

One could argue that this PR is non-controversial (reasoning: adding a file is not an endorsement of a specific tool).

And one could argue that this PR is "controversial" (reasoning: adding a yapf specific file to the repo is an endorsement of the yapf over more popular alternatives such as black).

However, I'm not sure that really matters for the policy discussion.

What matters is the general policy question raised:

The likelihood of merge for a PR submitted by person A should be a function of the technical PR content alone (as judged by reviewers in a technical review). More specifically it should be independent of the "status" of A - such as if A is a maintainer or not.

That's 100% non-controversial, right? :-)

I don't want to argue again about the merit of this change. I already expressed why I didn't think it was beneficial and that was ignored.

My point in #15479 (comment) is simply that I don't think maintainers should be merging their own PRs without review from other contributors.