Format all python code with black

[timokau]

Description

This PR formats all python code with black, the "uncompromising Python code formatter".

Motivation and Context

They state the benefits in their README:

Black is the uncompromising Python code formatter. By using it, you agree to cede control over minutiae of hand-formatting. In return, Black gives you speed, determinism, and freedom from pycodestyle nagging about formatting. You will save time and mental energy for more important matters.

Blackened code looks the same regardless of the project you're reading. Formatting becomes transparent after a while and you can focus on the content instead.

Black makes code review faster by producing the smallest diffs possible.

However there are downsides as well. I can think of two:

• it mangles git history a bit, since it will introduce one commit that will touch pretty much all the code.
• its a bit of an additional hurdle for new contributors

Personally, I do think that the benefits (uniform formatting, no nit-picking in reviews, not having to think about formatting when writing code) far outweigh the downsides. Especially at the early stage we are now, the downsides are comparatively low. There are not a lot of PRs to rebase and much of the code is intended to be improved in the future anyways.

Still, this PR is intended a basis of discussion first of all. What do you think?

A few relevant links:

• discussion on partial formatting
• discussion on HN

If we were to do this, we'd also configure travis to check the formatting and provide a pre-commit specification to automatically apply it.

How Has This Been Tested?

Black makes sure that the AST before formatting is equal to the AST after formatting.

Does this close/impact existing issues?

N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.

[coveralls]

Pull Request Test Coverage Report for Build 739

• 659 of 1198 (55.01%) changed or added relevant lines in 100 files are covered.
• 56 unchanged lines in 31 files lost coverage.
• Overall coverage decreased (-9.06%) to 49.786%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
csrank/choicefunction/baseline.py	0	1	0.0%
csrank/choicefunction/choice_functions.py	2	3	66.67%
csrank/choicefunction/fatelinear_choice.py	9	10	90.0%
csrank/choicefunction/fetalinear_choice.py	9	10	90.0%
csrank/choicefunction/ranknet_choice.py	12	13	92.31%
csrank/core/pairwise_svm.py	7	8	87.5%
csrank/dataset_reader/choicefunctions/letor_ranking_choice_dataset_reader.py	0	1	0.0%
csrank/dataset_reader/discretechoice/util.py	0	1	0.0%
csrank/discretechoice/cmpnet_discrete_choice.py	7	8	87.5%
csrank/discretechoice/ranknet_discrete_choice.py	7	8	87.5%

Files with Coverage Reduction	New Missed Lines	%
csrank/choicefunction/cmpnet_choice.py	1	84.78%
csrank/choicefunction/fatelinear_choice.py	1	92.86%
csrank/choicefunction/fetalinear_choice.py	1	92.86%
csrank/choicefunction/generalized_linear_model.py	1	87.23%
csrank/core/fate_network.py	1	68.78%
csrank/core/pairwise_svm.py	1	92.16%
csrank/dataset_reader/choicefunctions/mnist_choice_dataset_reader.py	1	13.11%
csrank/dataset_reader/discretechoice/mnist_discrete_choice_dataset_reader.py	1	10.42%
csrank/dataset_reader/dyadranking/sushi_dyad_ranking_dataset_reader.py	1	47.62%
csrank/dataset_reader/mnist_dataset_reader.py	1	28.0%

Totals
Change from base Build 738:	-9.06%
Covered Lines:	3719
Relevant Lines:	7470

---

💛 - Coveralls

[kiudee]

I am aware of black and also experimented with it a bit. In general, I am in favor of a consistent code format.

Could you look into how to make it a pre-commit hook, which only reformats the changed lines of a commit? That way we can make sure that any new code is formatted correctly, without obfuscating the true changes a commit made, because everything was reformatted.

[timokau]

I already looked into that a bit, that was the reason for posting the link to the discussion on partial formatting. I should've been a bit more explicit there.

The gist is that the black authors made an explicit design decision not to support partial formatting, i.e. only formatting / checking the lines that were changed. There are third party projects / hacks to support this anyway, but they're not perfect.

There's darken which (ab-)uses # fmt: off markers but apparently has some inconsistencies with blank lines.

Then there is black-macciato, which apparently just cuts out the relevant lines and passes them through black individually. Not sure how well that works, but it doesn't sound very robust.

A third option would be to gradually format some files and mark them with a specific keyword in a comment. Formatting would then only be checked for those files.

[kiudee]

That’s indeed inconvenient. I would be fine with solutions like darken or black-macciato, if the tools are reliable and if it is foreseeable that they will be maintained in the future.

The third option does sound like more of a hassle than simply nit-picking for every pull request, that the code format should be followed.

The fourth option of course being doing a complete reformat, with all the consequences (e.g. broken blame, future reformatting because of changes in black etc).

[timokau]

That’s indeed inconvenient. I would be fine with solutions like darken or black-macciato, if the tools are reliable and if it is foreseeable that they will be maintained in the future.

Its hard to judge if they will remain maintained. They both have very little activity. But given that they are pretty simple tools, maybe there just isn't much maintenance needed.

The third option does sound like more of a hassle than simply nit-picking for every pull request, that the code format should be followed.

The third option could also be automated, but I agree that it sort of just combines the "worst of both worlds" since it will still require a separate reformat commit per file.

The fourth option of course being doing a complete reformat, with all the consequences (e.g. broken blame, future reformatting because of changes in black etc).

I would prefer that option while it's still feasible. git blame can be somewhat recovered by using --ignore-rev. Future formatting changes should be minimal, according to the README:

What this means for you is that 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.

A significant downside of the incremental approaches is that they can be inconvenient for the user. It would require them to use our pre-commit hook or figure out how to apply partial formatting themselves. If we just format everything, they can choose whether to use the pre-commit hook or juts manually invoke black.

Of course doing nothing is also a valid option.

Should I look further into the incremental options (darken an macciato)?

[kiudee]

The fourth option of course being doing a complete reformat, with all the consequences (e.g. broken blame, future reformatting because of changes in black etc).

I would prefer that option while it's still feasible. git blame can be somewhat recovered by using --ignore-rev. Future formatting changes should be minimal, according to the README:

What this means for you is that 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.

A significant downside of the incremental approaches is that they can be inconvenient for the user. It would require them to use our pre-commit hook or figure out how to apply partial formatting themselves. If we just format everything, they can choose whether to use the pre-commit hook or juts manually invoke black.

Good points, after thinking about this a bit more and checking out different projects using pre-commit hooks, I am also in favor of this option.

What I think needs to be done:

• Set up pre-commit with a sensible configuration
• Document how to contribute to the project - also mentioning pre-commit/black (cf. flask’s CONTRIBUTING file)
• Make sure all other branches are merged before merging this change (to avoid conflicts)

[timokau]

I'll add CI & docs in follow-up PRs.