Proposal: Remove black from CI pipeline
After working with black
in the CI pipeline for a couple of months, I am increasingly skeptical of the value it is adding, and have encountered some situations where it has introduced unnecessary complications. My reasons are as follows:
-
The vast majority of the formatting changes that
black
makes are inconsequential and have no discernible impact on code functionality, quality or readability. -
We have a pipeline fail here: https://git.gfz-potsdam.de/gweather/shakyground2/-/jobs/69384 because
black
andflake8
are contradicting each other.black
insists that the following line should be formatted as:
tag = tag[tag.index("}") + 1 :]
while flake8
insists that it must be:
tag = tag[tag.index("}") + 1:]
This absolutely trivial issue is preventing a merge request from passing the CI pipeline - requiring time invested to find the solution.
I have also encountered problems myself using black
with type hinting, which prefers to change the perfectly valid Optional[x]
to Union[x, None]
, which will introduce an error if Union
has not been imported. Again, it is not an insurmountable problem, but it is an unnecessary one.
-
Other differences such as line-length requirements can be configured to avoid conflicts (as they are currently), but I can see circumstances where a contributor is unfamiliar with
black
and may run the standard commandblack ...
with the defaults, only to have it modify the code in a way that is incompatible with the black configuration in the CI. You can see this particular error from me in the commits to this MR: !13 (commits), and though this might have been my own error it did cost time to understand and fix the problem, but for no gain in terms of code quality. For an outside contributor, annoyances like this can be a barrier to engaging with a project. -
Quite simply, while the CI pipeline runs
flake8
thenblack
is redundant for actually catching problems of formatting and readability. I am not suggesting to abandon the use ofblack
entirely. As we have now begun to develop the code with one particular style I will continue to runblack
prior to submitting MRs and add this into the readme as an instruction to contributors to do the same. However, makingblack
an integral part of the CI pipeline to the extent that trivial formatting issues causing it to break is adding complications for no gain.
@marius @rizac @ds I would not remove this without agreement (though I don't know how to get around the problem in point #2 (closed)), but I would like to put forward my views and experiences on this, which may be relevant to you if you are using black
in other projects.