[Developer Experience] Implement black as code formatter

[vinitkumar]

To ensure the code looks consistent across all the files and remove all style related arguments from the code, I propose that we move to black as our official code formatter. Once decided, we also release instructions on how to configure it and even an example to add it as a pre-commit hook.

A lot of python projects and libraries have moved to black for formatting, and I think we should join the wagon too. I am curious to know what everyone thinks about it?

[marksweb]

I'm yet to ever use black. I do see it used a lot though in OSS. Typically at work we've only ever been in fairly small teams so it's easy to get everyone to follow the same kind of structure.

I can see the benefit of it on projects like this though. Even if it might completely destroy my preferred coding styles 😆

We could also put black and flake8 into pre-commit hooks so that they're being ran as people work rather than just when things hit github actions.

[marksweb]

For anyone wanting to explore what this would provide, for django-cms or any other project, I've been looking into hooks lately.

Starting with... https://pre-commit.com/

There's a python package you can install, pip install pre-commit

Then create a file in the root of your project called .pre-commit-config.yaml

To simply run black populate that file with

repos:
-   repo: https://github.com/psf/black
    rev: 21.9b0
    hooks:
    -   id: black

However you could run isort, flake8, black and various other helpful tools to keep the code clean & tidy.

I'm currently investigating this pre-commit setup, which as of yet, doesn't run black, but does run other useful tools;

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
  rev: v3.2.0
  hooks:
    - id: check-added-large-files
    - id: check-ast
    - id: check-case-conflict
    - id: check-executables-have-shebangs
    - id: check-merge-conflict
    - id: debug-statements
    - id: end-of-file-fixer
    - id: trailing-whitespace

- repo: https://github.com/pycqa/isort
  rev: 5.8.0
  hooks:
    - id: isort
      name: isort (python)

- repo: https://github.com/Lucas-C/pre-commit-hooks-safety
  rev: v1.2.2
  hooks:
    - id: python-safety-dependencies-check

- repo: https://gitlab.com/pycqa/flake8
  rev: 3.8.4
  hooks:
    - id: flake8
      additional_dependencies: [
          "flake8-bugbear",
          "flake8-comprehensions",
          "flake8-mutable",
          "flake8-print",
          "flake8-simplify",
      ]

[vinitkumar]

pre-commit is a great tool and I have seen many people using it.

[changeling]

Just adding a vote for black. I fought tooth and nail against it for a long while, trying to keep my own habits in place.

After finally succumbing a couple of years ago, I have to say it's been amazing, both for my own projects and for collaborative work. The sheer number of open source projects basing around it make PRs so much easier once i caved.

[Aiky30]

My biggest issue with installing code formatters on existing projects is that the entire codebase is changed in one commit and tools such as Blame become useless to track changes and help bugfix. This will also make porting changes to v4 more difficult so of course I will complain. :-) Haha.

I see a place for tools like this on new projects. If there was a way to force new changes to respect the rules that would help as it is implemented on each change and won't affect the code change history.

[marksweb]

That's a great point. Could do with introducing it somewhere without a run on all files to see how it works.
I think it'd just run on changes going forward from what I've read, but need to try it out.

[marksweb]

Alright, I've just tried to take a look at black and understand how you can make it work for a project... but you can't. It seems you cannot configure it beyond some basics like line length.

Giving it a run on django-bleach to see how it's code looks and I've gone off it already.

It's ruined the consistency of my code.

I'd rather we didn't use this package and instead just had flake8 running how we want as that will keep code formatted well, while allowing a little freedom.

[marksweb]

I may have been a little too quick to judge.

It seems there are ways to work around this type of reformatting.

% black --code "dict(a=1,b=2,c=3)"
dict(a=1, b=2, c=3)
% black --code "dict(a=1,b=2,c=3,)"
dict(
    a=1,
    b=2,
    c=3,
)

[MacLake]

As I haven’t contributed to the django CMS code so far, I don’t feel in the position to ask for any direction to go in this matter, I can only share my experience: Until last year I just pressed sometimes Ctrl-Alt-L on PyCharm in order to do some automatic formatting, then I read a comparison of autopep8, black and yapf. It seemed to be a fair comparision. If I can find it again, I’ll let you know.

autopep8 seemed to be not so good, black and yapf with Facebook style yielded similar results. Some people like it that black is as good as not configurable, because you don’t have to think about it, and you avoid discussions about the configuration in a team. yapf is slower than black, but you can configure many parameters, that’s the main reason why I’ve chosen it for my code, and so far I’m satisfied with the result. Also for yapf there are markers that tell it not to touch code fragments that it would mess up otherwise. Btw: Unlike me the author of the comparison chose black, but also said that the other formatters were good.

yapf comes with four standard configs: yapf, pep8, Google, Facebook. It’s a project by Google, and apparently also used by Facebook. So if your concern of using black is that it’s hardly configurable, you might also want to give yapf a try.

[macolo]

@marksweb made an interesting point today in the TC meeting:

Would be worth mentioning my pre-commit PR. If people are happy with what I've included, we can spread this to the other repos; django-cms/djangocms-bootstrap4#142

[mazulo]

I really like the idea of running either black or blue. The drawback of having it kinda of messing with the git history is really a pain, but I think that we can overcome this problem by setting up it to be run in some parts of the code, like piece by piece instead of the whole project at once. This way we can iterate over it in a better way. There's also some ways where we can integrate it in our CI process (however it will need some more work to be done. Just would love to hear what other folks think about it.

[marksweb]

We discussed this last year and he consensus was that we don't like the way black works. We decided that with a pre-commit config including flake8, isort & a few other hooks we can maintain quality code.

For a specific example, in my screenshot above, running black actually makes the code less readable. Model field attributes go from. being per-line to one long line. I find it incredibly easy to read models where the attributes are split by line, whereas scrolling through 1 line takes much longer.

If you've been reading through the project and had issue with the format, can you cite any examples?

[jrief]

As mentioned in our Friday meetings, I disagree with some settings used in black.

Now that the Django project uses black, they reduced the maximum line length from 119 to 88. This in my opinion
decreases the readability of code, because either logical lines have to be split or variable names will be shortened.
Most of us developers have wide screens and can fit 119 characters without having to scroll horizontally.

Something else, but less important is quoting. Since Python does not distinguish between single and double
quotes, I use them to distinguish strings semantically. Strings intended to be read by humans are quoted as "…",
while strings intended to be used by the app (keys, etc.), are quoted as '…'.

I fully agree that some parts of the code shall be reformatted, for instance the way we declare models and forms.

[changeling]

For what it's worth, you can set line length in black with the -l or --line-length flag.

You can also disable the quoting behavior with the -S or --skip-string-normalization flag.

As an aside, though, the point of black is specifically to remove personal style from shared projects. I'd suggest this is an necessary step for any open sourced collaborative project.

[mazulo]

Exactly, that along with the approach of just formatting some parts of the code at once (instead of all of it) we can improve the code readability one step at a time.

[DmytroLitvinov]

Just found out that conversation and thinking about raising a new discussion to use ruff instead

[Aiky30]

I've been using Ruff for sometime now and agree it's so much faster locally and in CI.

[marksweb]

Yeah I've been meaning to raise it. We've been implementing the ruff linting, so we should do ruff formatting as well given the compatibility with black.