Opened 2 years ago

Closed 2 years ago

Last modified 2 years 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 by Jeff Triplett, 2 years ago

Description: modified (diff)

comment:2 by Carlton Gibson, 2 years ago

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 by Jeff Triplett, 2 years ago

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

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.

Version 0, edited 2 years ago by Jeff Triplett (next)

comment:4 by Carlton Gibson, 2 years ago

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 by Mariusz Felisiak, 2 years ago

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 by Jeff Triplett, 2 years ago

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 by Carlton Gibson, 2 years ago

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 2 years ago by Carlton Gibson (previous) (diff)

in reply to:  7 ; comment:8 by Jeff Triplett, 2 years ago

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.

in reply to:  8 comment:9 by Oliver Andrich, 2 years ago

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 by Carlton Gibson, 2 years ago

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 by gradyy, 2 years ago

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

comment:12 by Mariusz Felisiak, 2 years ago

Severity: NormalRelease blocker

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

comment:13 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

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