[Django] #34085: Black shouldn't format non-Python files
Django
-------------------------------------+-------------------------------------
Reporter: Jeff | Owner: nobody
Triplett |
Type: Bug | Status: new
Component: Core | Version: 4.1
(Management commands) |
Severity: Normal | Keywords: startproject black
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/34085>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Component: Core (Management | Version: 4.1
commands) |
Keywords: startproject black | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Type: Bug | Status: closed
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution: needsinfo
Keywords: startproject black | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jeff Triplett):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Oliver Andrich):
Replying to [comment:8 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:9>
Django
-------------------------------------+-------------------------------------
| Gibson
Type: Bug | Status: assigned
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* owner: nobody => Carlton Gibson
* needs_docs: 0 => 1
* has_patch: 0 => 1
Comment:
There's a [https://github.com/django/django/pull/16192 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:10>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (Management | Version: 4.1
commands) |
Severity: Normal | Resolution:
Keywords: startproject black | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: Carlton Gibson => gradyy
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0
Comment:
New PR https://github.com/django/django/pull/16201
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: gradyy
Type: Bug | Status: assigned
Component: Core (Management | Version: 4.1
commands) |
Keywords: startproject black | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
Comment:
Marking as a release blocker as it's a bug in
d113b5a837f726d1c638d76c4e88445e6cd59fd5.
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: gradyy
Type: Bug | Status: assigned
Component: Core (Management | Version: 4.1
commands) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: gradyy
Component: Core (Management | Version: 4.1
commands) |
Keywords: startproject black | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"5c2c7277d4554db34c585477b269bb1acfcbbe56" 5c2c7277]:
{{{
#!CommitTicketReference repository=""
revision="5c2c7277d4554db34c585477b269bb1acfcbbe56"
Fixed #34085 -- Made management commands don't use black for non-Python
files.
Bug in d113b5a837f726d1c638d76c4e88445e6cd59fd5.
Co-authored-by: programmylife <acm...@gmail.com>
Co-authored-by: Carlton Gibson <carlton...@noumenal.es>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Jeff Triplett | Owner: gradyy
Type: Bug | Status: closed
Component: Core (Management | Version: 4.1
commands) |
Severity: Release blocker | Resolution: fixed
Keywords: startproject black | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"84814412a032742e5ccef576b24dbaa0ef25fe06" 84814412]:
{{{
#!CommitTicketReference repository=""
revision="84814412a032742e5ccef576b24dbaa0ef25fe06"
[4.1.x] Fixed #34085 -- Made management commands don't use black for non-
Python files.
Bug in d113b5a837f726d1c638d76c4e88445e6cd59fd5.
Co-authored-by: programmylife <acm...@gmail.com>
Co-authored-by: Carlton Gibson <carlton...@noumenal.es>
Backport of 5c2c7277d4554db34c585477b269bb1acfcbbe56 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34085#comment:15>