Browse Source

Refs #33476 -- Made management commands use black.

Run black on generated files, if it is available on PATH.
Carlton Gibson 3 years ago
parent
commit
d113b5a837

+ 5 - 0
django/core/management/commands/makemigrations.py

@@ -6,6 +6,7 @@ from itertools import takewhile
 from django.apps import apps
 from django.conf import settings
 from django.core.management.base import BaseCommand, CommandError, no_translations
+from django.core.management.utils import run_formatters
 from django.db import DEFAULT_DB_ALIAS, OperationalError, connections, router
 from django.db.migrations import Migration
 from django.db.migrations.autodetector import MigrationAutodetector
@@ -88,6 +89,7 @@ class Command(BaseCommand):
 
     @no_translations
     def handle(self, *app_labels, **options):
+        self.written_files = []
         self.verbosity = options["verbosity"]
         self.interactive = options["interactive"]
         self.dry_run = options["dry_run"]
@@ -276,6 +278,7 @@ class Command(BaseCommand):
                     migration_string = writer.as_string()
                     with open(writer.path, "w", encoding="utf-8") as fh:
                         fh.write(migration_string)
+                        self.written_files.append(writer.path)
                 elif self.verbosity == 3:
                     # Alternatively, makemigrations --dry-run --verbosity 3
                     # will log the migrations rather than saving the file to
@@ -286,6 +289,7 @@ class Command(BaseCommand):
                         )
                     )
                     self.log(writer.as_string())
+        run_formatters(self.written_files)
 
     def handle_merge(self, loader, conflicts):
         """
@@ -382,6 +386,7 @@ class Command(BaseCommand):
                     # Write the merge migrations file to the disk
                     with open(writer.path, "w", encoding="utf-8") as fh:
                         fh.write(writer.as_string())
+                    run_formatters([writer.path])
                     if self.verbosity > 0:
                         self.log("\nCreated new merge migration %s" % writer.path)
                         if self.scriptable:

+ 2 - 0
django/core/management/commands/squashmigrations.py

@@ -3,6 +3,7 @@ import os
 from django.apps import apps
 from django.conf import settings
 from django.core.management.base import BaseCommand, CommandError
+from django.core.management.utils import run_formatters
 from django.db import DEFAULT_DB_ALIAS, connections, migrations
 from django.db.migrations.loader import AmbiguityError, MigrationLoader
 from django.db.migrations.migration import SwappableTuple
@@ -220,6 +221,7 @@ class Command(BaseCommand):
             )
         with open(writer.path, "w", encoding="utf-8") as fh:
             fh.write(writer.as_string())
+        run_formatters([writer.path])
 
         if self.verbosity > 0:
             self.stdout.write(

+ 5 - 1
django/core/management/templates.py

@@ -12,7 +12,7 @@ from urllib.request import build_opener
 import django
 from django.conf import settings
 from django.core.management.base import BaseCommand, CommandError
-from django.core.management.utils import handle_extensions
+from django.core.management.utils import handle_extensions, run_formatters
 from django.template import Context, Engine
 from django.utils import archive
 from django.utils.version import get_docs_version
@@ -80,6 +80,7 @@ class TemplateCommand(BaseCommand):
         )
 
     def handle(self, app_or_project, name, target=None, **options):
+        self.written_files = []
         self.app_or_project = app_or_project
         self.a_or_an = "an" if app_or_project == "app" else "a"
         self.paths_to_remove = []
@@ -200,6 +201,7 @@ class TemplateCommand(BaseCommand):
                 else:
                     shutil.copyfile(old_path, new_path)
 
+                self.written_files.append(new_path)
                 if self.verbosity >= 2:
                     self.stdout.write("Creating %s" % new_path)
                 try:
@@ -222,6 +224,8 @@ class TemplateCommand(BaseCommand):
                 else:
                     shutil.rmtree(path_to_remove)
 
+        run_formatters(self.written_files)
+
     def handle_template(self, template, subdir):
         """
         Determine where the app or project templates are.

+ 13 - 0
django/core/management/utils.py

@@ -1,5 +1,7 @@
 import fnmatch
 import os
+import shutil
+import subprocess
 from pathlib import Path
 from subprocess import run
 
@@ -153,3 +155,14 @@ def is_ignored_path(path, ignore_patterns):
         )
 
     return any(ignore(pattern) for pattern in normalize_path_patterns(ignore_patterns))
+
+
+def run_formatters(written_files):
+    """
+    Run the black formatter on the specified files.
+    """
+    if black_path := shutil.which("black"):
+        subprocess.run(
+            [black_path, "--fast", "--", *written_files],
+            capture_output=True,
+        )

+ 18 - 0
docs/ref/django-admin.txt

@@ -2050,6 +2050,24 @@ distribution. It enables tab-completion of ``django-admin`` and
 
 See :doc:`/howto/custom-management-commands` for how to add customized actions.
 
+Black formatting
+----------------
+
+.. versionadded:: 4.1
+
+The Python files created by :djadmin:`startproject`, :djadmin:`startapp`,
+:djadmin:`makemigrations`, and :djadmin:`squashmigrations` are formatted using
+the ``black`` command if it is present on your ``PATH``.
+
+If you have ``black`` globally installed, but do not wish it used for the
+current project, you can set the ``PATH`` explicitly::
+
+    PATH=path/to/venv/bin django-admin makemigrations
+
+For commands using ``stdout`` you can pipe the output to ``black`` if needed::
+
+    django-admin inspectdb | black -
+
 ==========================================
 Running management commands from your code
 ==========================================

+ 4 - 0
docs/releases/4.1.txt

@@ -226,6 +226,10 @@ Management Commands
 * The new :option:`migrate --prune` option allows deleting nonexistent
   migrations from the ``django_migrations`` table.
 
+* Python files created by :djadmin:`startproject`, :djadmin:`startapp`,
+  :djadmin:`makemigrations`, and :djadmin:`squashmigrations` are now formatted
+  using the ``black`` command if it is present on your ``PATH``.
+
 Migrations
 ~~~~~~~~~~
 

+ 3 - 3
tests/admin_scripts/custom_templates/project_template/manage.py-tpl

@@ -1,6 +1,6 @@
 # The manage.py of the {{ project_name }} test project
 
 # template context:
-project_name = '{{ project_name }}'
-project_directory = '{{ project_directory }}'
-secret_key = '{{ secret_key }}'
+project_name = "{{ project_name }}"
+project_directory = "{{ project_directory }}"
+secret_key = "{{ secret_key }}"

+ 15 - 6
tests/admin_scripts/tests.py

@@ -41,6 +41,8 @@ custom_templates_dir = os.path.join(os.path.dirname(__file__), "custom_templates
 
 SYSTEM_CHECK_MSG = "System check identified no issues"
 
+HAS_BLACK = shutil.which("black")
+
 
 class AdminScriptTestCase(SimpleTestCase):
     def setUp(self):
@@ -732,7 +734,10 @@ class DjangoAdminSettingsDirectory(AdminScriptTestCase):
         with open(os.path.join(app_path, "apps.py")) as f:
             content = f.read()
             self.assertIn("class SettingsTestConfig(AppConfig)", content)
-            self.assertIn("name = 'settings_test'", content)
+            self.assertIn(
+                'name = "settings_test"' if HAS_BLACK else "name = 'settings_test'",
+                content,
+            )
 
     def test_setup_environ_custom_template(self):
         "directory: startapp creates the correct directory with a custom template"
@@ -754,7 +759,7 @@ class DjangoAdminSettingsDirectory(AdminScriptTestCase):
         with open(os.path.join(app_path, "apps.py"), encoding="utf8") as f:
             content = f.read()
             self.assertIn("class こんにちはConfig(AppConfig)", content)
-            self.assertIn("name = 'こんにちは'", content)
+            self.assertIn('name = "こんにちは"' if HAS_BLACK else "name = 'こんにちは'", content)
 
     def test_builtin_command(self):
         """
@@ -2614,8 +2619,8 @@ class StartProject(LiveServerTestCase, AdminScriptTestCase):
         test_manage_py = os.path.join(testproject_dir, "manage.py")
         with open(test_manage_py) as fp:
             content = fp.read()
-            self.assertIn("project_name = 'another_project'", content)
-            self.assertIn("project_directory = '%s'" % testproject_dir, content)
+            self.assertIn('project_name = "another_project"', content)
+            self.assertIn('project_directory = "%s"' % testproject_dir, content)
 
     def test_no_escaping_of_project_variables(self):
         "Make sure template context variables are not html escaped"
@@ -2880,11 +2885,15 @@ class StartApp(AdminScriptTestCase):
         with open(os.path.join(app_path, "apps.py")) as f:
             content = f.read()
             self.assertIn("class NewAppConfig(AppConfig)", content)
+            if HAS_BLACK:
+                test_str = 'default_auto_field = "django.db.models.BigAutoField"'
+            else:
+                test_str = "default_auto_field = 'django.db.models.BigAutoField'"
+            self.assertIn(test_str, content)
             self.assertIn(
-                "default_auto_field = 'django.db.models.BigAutoField'",
+                'name = "new_app"' if HAS_BLACK else "name = 'new_app'",
                 content,
             )
-            self.assertIn("name = 'new_app'", content)
 
 
 class DiffSettings(AdminScriptTestCase):

+ 30 - 6
tests/migrations/test_commands.py

@@ -2,6 +2,7 @@ import datetime
 import importlib
 import io
 import os
+import shutil
 import sys
 from unittest import mock
 
@@ -28,6 +29,8 @@ from .models import UnicodeModel, UnserializableModel
 from .routers import TestRouter
 from .test_base import MigrationTestBase
 
+HAS_BLACK = shutil.which("black")
+
 
 class MigrateTests(MigrationTestBase):
     """
@@ -1524,8 +1527,12 @@ class MakeMigrationsTests(MigrationTestBase):
 
                 # Remove all whitespace to check for empty dependencies and operations
                 content = content.replace(" ", "")
-                self.assertIn("dependencies=[\n]", content)
-                self.assertIn("operations=[\n]", content)
+                self.assertIn(
+                    "dependencies=[]" if HAS_BLACK else "dependencies=[\n]", content
+                )
+                self.assertIn(
+                    "operations=[]" if HAS_BLACK else "operations=[\n]", content
+                )
 
     @override_settings(MIGRATION_MODULES={"migrations": None})
     def test_makemigrations_disabled_migrations_for_app(self):
@@ -1661,6 +1668,13 @@ class MakeMigrationsTests(MigrationTestBase):
                 "0003_merge_0002_conflicting_second_0002_second.py",
             )
             self.assertIs(os.path.exists(merge_file), True)
+            with open(merge_file, encoding="utf-8") as fp:
+                content = fp.read()
+            if HAS_BLACK:
+                target_str = '("migrations", "0002_conflicting_second")'
+            else:
+                target_str = "('migrations', '0002_conflicting_second')"
+            self.assertIn(target_str, content)
         self.assertIn("Created new merge migration %s" % merge_file, out.getvalue())
 
     @mock.patch("django.db.migrations.utils.datetime")
@@ -2252,7 +2266,9 @@ class MakeMigrationsTests(MigrationTestBase):
             # generate an initial migration
             migration_name_0001 = "my_initial_migration"
             content = cmd("0001", migration_name_0001)
-            self.assertIn("dependencies=[\n]", content)
+            self.assertIn(
+                "dependencies=[]" if HAS_BLACK else "dependencies=[\n]", content
+            )
 
             # importlib caches os.listdir() on some platforms like macOS
             # (#23850).
@@ -2262,11 +2278,15 @@ class MakeMigrationsTests(MigrationTestBase):
             # generate an empty migration
             migration_name_0002 = "my_custom_migration"
             content = cmd("0002", migration_name_0002, "--empty")
+            if HAS_BLACK:
+                template_str = 'dependencies=[\n("migrations","0001_%s"),\n]'
+            else:
+                template_str = "dependencies=[\n('migrations','0001_%s'),\n]"
             self.assertIn(
-                "dependencies=[\n('migrations','0001_%s'),\n]" % migration_name_0001,
+                template_str % migration_name_0001,
                 content,
             )
-            self.assertIn("operations=[\n]", content)
+            self.assertIn("operations=[]" if HAS_BLACK else "operations=[\n]", content)
 
     def test_makemigrations_with_invalid_custom_name(self):
         msg = "The migration name must be a valid Python identifier."
@@ -2606,7 +2626,11 @@ class SquashMigrationsTests(MigrationTestBase):
             )
             with open(squashed_migration_file, encoding="utf-8") as fp:
                 content = fp.read()
-                self.assertIn("        ('migrations', '0001_initial')", content)
+                if HAS_BLACK:
+                    test_str = '        ("migrations", "0001_initial")'
+                else:
+                    test_str = "        ('migrations', '0001_initial')"
+                self.assertIn(test_str, content)
                 self.assertNotIn("initial = True", content)
         out = out.getvalue()
         self.assertNotIn(" - 0001_initial", out)

+ 1 - 0
tests/requirements/py3.txt

@@ -3,6 +3,7 @@ asgiref >= 3.4.1
 argon2-cffi >= 16.1.0
 backports.zoneinfo; python_version < '3.9'
 bcrypt
+black
 docutils
 geoip2
 jinja2 >= 2.9.2