From d113b5a837f726d1c638d76c4e88445e6cd59fd5 Mon Sep 17 00:00:00 2001 From: Carlton Gibson Date: Tue, 8 Feb 2022 12:38:43 +0100 Subject: [PATCH] Refs #33476 -- Made management commands use black. Run black on generated files, if it is available on PATH. --- .../management/commands/makemigrations.py | 5 +++ .../management/commands/squashmigrations.py | 2 ++ django/core/management/templates.py | 6 +++- django/core/management/utils.py | 13 +++++++ docs/ref/django-admin.txt | 18 ++++++++++ docs/releases/4.1.txt | 4 +++ .../project_template/manage.py-tpl | 6 ++-- tests/admin_scripts/tests.py | 21 +++++++---- tests/migrations/test_commands.py | 36 +++++++++++++++---- tests/requirements/py3.txt | 1 + 10 files changed, 96 insertions(+), 16 deletions(-) diff --git a/django/core/management/commands/makemigrations.py b/django/core/management/commands/makemigrations.py index 5afa209136..8938fb6309 100644 --- a/django/core/management/commands/makemigrations.py +++ b/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: diff --git a/django/core/management/commands/squashmigrations.py b/django/core/management/commands/squashmigrations.py index aafa5e7bcc..2d6e0ebfa3 100644 --- a/django/core/management/commands/squashmigrations.py +++ b/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( diff --git a/django/core/management/templates.py b/django/core/management/templates.py index 58005c23ed..72db9651b0 100644 --- a/django/core/management/templates.py +++ b/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. diff --git a/django/core/management/utils.py b/django/core/management/utils.py index c12d90f6ae..e3e1122409 100644 --- a/django/core/management/utils.py +++ b/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, + ) diff --git a/docs/ref/django-admin.txt b/docs/ref/django-admin.txt index a60c538331..793461c265 100644 --- a/docs/ref/django-admin.txt +++ b/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 ========================================== diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt index 6dd36e498c..88d283ff65 100644 --- a/docs/releases/4.1.txt +++ b/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 ~~~~~~~~~~ diff --git a/tests/admin_scripts/custom_templates/project_template/manage.py-tpl b/tests/admin_scripts/custom_templates/project_template/manage.py-tpl index d9843c433f..42a1fa6684 100755 --- a/tests/admin_scripts/custom_templates/project_template/manage.py-tpl +++ b/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 }}" diff --git a/tests/admin_scripts/tests.py b/tests/admin_scripts/tests.py index 3d5902f768..9d2ca839d5 100644 --- a/tests/admin_scripts/tests.py +++ b/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): diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index b3d9972b0c..4d7918238e 100644 --- a/tests/migrations/test_commands.py +++ b/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) diff --git a/tests/requirements/py3.txt b/tests/requirements/py3.txt index 7ce1ca9b2e..9092d4b7de 100644 --- a/tests/requirements/py3.txt +++ b/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