Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#34085 closed Bug (fixed)

Black shouldn't format non-Python files

Reported by: Jeff Triplett Owned by: gradyy
Component: Core (Management commands) Version: 4.1
Severity: Release blocker Keywords: startproject black
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (15)

comment:1 Changed 4 months ago by Jeff Triplett

Description: modified (diff)

comment:2 Changed 4 months ago by Carlton Gibson

Resolution: needsinfo
Status: newclosed

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.

comment:3 Changed 4 months ago by 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.

Last edited 4 months ago by Jeff Triplett (previous) (diff)

comment:4 Changed 4 months ago by Carlton Gibson

Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted

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.

comment:5 Changed 4 months ago by 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.

comment:6 Changed 4 months ago by 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.

comment:7 Changed 4 months ago by 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...)

Last edited 4 months ago by Carlton Gibson (previous) (diff)

comment:8 in reply to:  7 ; Changed 4 months ago by 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.

comment:9 in reply to:  8 Changed 4 months ago by 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.

comment:10 Changed 4 months ago by Carlton Gibson

Has patch: set
Needs documentation: set
Needs tests: set
Owner: changed from nobody to Carlton Gibson
Patch needs improvement: set
Status: newassigned

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.

comment:11 Changed 3 months ago by gradyy

Needs documentation: unset
Needs tests: unset
Owner: changed from Carlton Gibson to gradyy
Patch needs improvement: unset

comment:12 Changed 3 months ago by Mariusz Felisiak

Severity: NormalRelease blocker

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

comment:13 Changed 3 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:14 Changed 3 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

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@…>

comment:15 Changed 3 months ago by 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.

Note: See TracTickets for help on using tickets.
Back to Top