Black shouldn't format non-Python files

[Jeff Triplett]

Oliver Andrich on Twitter told me that the requirements.in file in my startproject was adding spaces after being processed. After a bit more digging it appears that Black is being run against non-Python files like requirements.in, which is a common file pather used with pip-tools and pip-compile to create a locked requirements.txt. ​https://twitter.com/oliverandrich/status/1579872988891328513

Ideally, Black should not run on non-Python files <or> it'd be nice to have a better exclude option as Carlton pointed out. See: ​https://twitter.com/carltongibson/status/1579888108010868738

While I am a fan of Black, I personally think we should have some type of --skip-black or --skip-formatters because it's disruptive to have Black installed globally and have this be opt-in by default.

[Carlton Gibson]

Hmm. This doesn't reproduce for me locally, so we're missing something... 🤔

Last login: Tue Oct 11 10:24:11 on ttys000
carlton@Carltons-MacBook-Pro ~ % mktmpenv 
...
(tmp-485998ddadf9b85) carlton@Carltons-MacBook-Pro tmp-485998ddadf9b85 % pip install Django
...
Successfully installed Django-4.1.2 asgiref-3.5.2 sqlparse-0.4.3
(tmp-485998ddadf9b85) carlton@Carltons-MacBook-Pro tmp-485998ddadf9b85 % django-admin startproject \
    --extension=ini,py,yml \
    --template=https://github.com/jefftriplett/django-startproject/archive/main.zip \
    example_project
(tmp-485998ddadf9b85) carlton@Carltons-MacBook-Pro tmp-485998ddadf9b85 % cat example_project/requirements/requirements.in 
django-click
Django<4.2
environs[django]
psycopg2-binary
whitenoise

black
django-test-plus
ipdb
model-bakery
pip-tools
pytest
pytest-cov
pytest-django
(tmp-485998ddadf9b85) carlton@Carltons-MacBook-Pro tmp-485998ddadf9b85 % black --version
black, version 21.10b0

Do you think Oliver could dig in a bit to spot where the issue is coming up? 🤔

While I am a fan of Black, I personally think we should have some type of --skip-black or --skip-formatters because it's disruptive to have Black installed globally and have this be opt-in by default.

This was discussed (at some length) during the development of the feature. It was decided against. Rather set the PATH explicitly if you have black installed globally but do not want to apply it for a particular invocation.

[Jeff Triplett]

Adding my repo in case it's useful: ​https://github.com/jefftriplett/django-startproject#wrench-install

I do have Black installed globally with pipx in case that's the missing link.

The repo isn't doing anything fancy, but it does have a requirements.in file that is repeatable.

$ django-admin startproject \
    --extension=ini,py,yml \
    --template=https://github.com/jefftriplett/django-startproject/archive/main.zip \
    example_project

*grumble* Please reconsider making assumptions about code formatting based on what people have installed globally. We have pre-commit and other tools which solve this issue explicitly and have a bit more granularly than "just run Black" by default.

[Carlton Gibson]

OK, I got it by updating black: upgraded package black from 21.10b0 to 22.10.0

I'll accept so we can investigate what happened there. Thanks.

[Mariusz Felisiak]

As far as I'm aware, excluding .ini files is something that should be controlled by a local Black configuration, not by Django itself.

[Jeff Triplett]

The .ini file is a red herring here from my example. It's a .in (no extra i) that Django seems to be passing through to Black somehow. I have used Black for years and I have never had either file extension processed by Black.

[Carlton Gibson]

Hey Jeff.

It's a .in (no extra i) that Django seems to be passing through to Black somehow. I have used Black for years and I have never had either file extension processed by Black.

So this behaviour changed somewhere between black 21.10b0 and 22.10.0 — I'm wondering who you know who might be able to point to where that was?
Django passes the written files to black (.png .txt files and so on, as well as .py). Black handles these well, and was doing the same for .in but not now. So 🤔

It looks like we might be able to pass the project dir here with this kind of diff:

(django) carlton@Carltons-MacBook-Pro tests % git diff
diff --git a/django/core/management/templates.py b/django/core/management/templates.py
index 71ee0a6ce8..5ee3664f77 100644
--- a/django/core/management/templates.py
+++ b/django/core/management/templates.py
@@ -232,7 +232,7 @@ class TemplateCommand(BaseCommand):
                 else:
                     shutil.rmtree(path_to_remove)
 
-        run_formatters(self.written_files, **formatter_paths)
+        run_formatters([top_dir], **formatter_paths)
 
     def handle_template(self, template, subdir):
         """

Then Black goes back to not processing the .in file. It looks as if directories are walked, and filtered for whether black thinks it can process them, whereas explicit files are attempted regardless. (It just so happens your .in file can be parsed as Python...)

[Jeff Triplett]

Django passes the written files to black (.png .txt files and so on, as well as .py).

Right, that's the issue. If you run Black on the folder it will ignore files that don't have a common py* extension.

If we pass a filename to Black to format the file then it will try to format the file which seems to be the existing behavior. Unless we filter out only known python extensions, I think your folder patch is the better default behavior. Otherwise, Django is going to break a bunch of requirements files for other projects and have other strange behavior for file style that are just python enough that they can be reformatted.

[Oliver Andrich]

Replying to Jeff Triplett:

If we pass a filename to Black to format the file then it will try to format the file which seems to be the existing behavior. Unless we filter out only known python extensions, I think your folder patch is the better default behavior. Otherwise, Django is going to break a bunch of requirements files for other projects and have other strange behavior for file style that are just python enough that they can be reformatted.

I think it is essential to skip the none-python files. I started to built my own starter based on Jeff's. I prefer poetry for that, and as the requirements.in is broken, my pyproject.toml is broken after using my unreleased template too.

[Carlton Gibson]

There's a ​draft PR here that adds the patch and begins a regression test. We will use it as an example (and aim to complete) for the "Getting started contributing to Django" sprints workshop at DjangoCon this weekend.

[gradyy]

New PR ​https://github.com/django/django/pull/16201

[Mariusz Felisiak]

Marking as a release blocker as it's a bug in d113b5a837f726d1c638d76c4e88445e6cd59fd5.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 5c2c7277:

Fixed #34085 -- Made management commands don't use black for non-Python files.

Bug in d113b5a837f726d1c638d76c4e88445e6cd59fd5.

Co-authored-by: programmylife <acmshar@…>
Co-authored-by: Carlton Gibson <carlton.gibson@…>

[Mariusz Felisiak <felisiak.mariusz@…>]

In 84814412:

[4.1.x] Fixed #34085 -- Made management commands don't use black for non-Python files.

Bug in d113b5a837f726d1c638d76c4e88445e6cd59fd5.

Co-authored-by: programmylife <acmshar@…>
Co-authored-by: Carlton Gibson <carlton.gibson@…>

Backport of 5c2c7277d4554db34c585477b269bb1acfcbbe56 from main.