easybuilders / easybuild-framework

EasyBuild is a software installation framework in Python that allows you to install software in a structured and robust way.
https://easybuild.io
GNU General Public License v2.0
148 stars 201 forks source link

use Black for code formatting #3165

Open smoors opened 4 years ago

smoors commented 4 years ago

from the website https://black.readthedocs.io/en/stable/ "Black makes code review faster by producing the smallest diffs possible. 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."

using Black would take away all the review comments and discussions about code formatting, which are often subjective and dependent on the preference of the reviewer.

formatting with Black could even be done on all PRs automatically, so the contributor does not even have to do anything.

Black also has a few options to relax the constraints a bit if desired: -changing the max line length to 120 chars (as is the case now) -ignoring changes to single and double quotes with --skip-string-normalization (by default, black uses double quotes everywhere).

the idea would be to start 'blackening' the framework code and test if everything works ok, next require Black formatting for all new PRs.

shahzebsiddiqui commented 4 years ago

i agree with @smoors i think i informed @boegel about this. I actually got this working as part of github workflow see https://github.com/HPC-buildtest/buildtest-framework/blob/devel/.github/workflows/black.yml

It works like a charm :)

Flamefire commented 4 years ago

It works like a charm :)

What exactly does it do? For me it looks like it simply runs black which may reformat code inside the CI working dir which has no impact on CI results or the actual code.

What is that CI action intended to do? I'd expect something like "check for style errors and bail out if any found" similar to what we have with Hound, although that is incomplete.

ocaisa commented 4 years ago

You can configure it so that it reports an error code (I use this on another project):

black --check .

you could also trigger

black --diff

if you catch an error code

ocaisa commented 4 years ago

But you are right @Flamefire , you would need people to install a pre-commit hook if you wanted that formatting automatically

shahzebsiddiqui commented 4 years ago

@ocaisa @Flamefire @boegel we now have a solution for getting black to work using hooks. See https://github.com/HPC-buildtest/buildtest-framework/issues/174 for more details.

ocaisa commented 4 years ago

The BDFL didn't seem to keen in https://github.com/easybuilders/easybuild-easyblocks/pull/1948#discussion_r376714895

Flamefire commented 4 years ago

@shahzebsiddiqui That's what @ocaisa said in https://github.com/easybuilders/easybuild-framework/issues/3165#issuecomment-581473270 People need to actively install it. Anyhow this is 2 steps to far: First there need to be a decision on a formatter and a check over all files on CI. Currently there isn't even a flake8 check so many issues currently exist in the code base. A formatter can solve some of them automatically but the EasyBuild team needs to make a call. As I wrote in https://github.com/easybuilders/easybuild-easyblocks/pull/1948 the alternative to black is yapf which allows more control. My usual approach is to use such a formatter and tune the config to make the least changes to the current code base. But in the end I don't really care: Any formatter will do