Fixed #24550 -- Added migration operation description to sqlmigrate output

Thanks Tim Graham for the review.

django/db/migrations/migration.py

@@ -91,13 +91,16 @@ class Migration(object):
         for operation in self.operations:
             # If this operation cannot be represented as SQL, place a comment
             # there instead
-            if collect_sql and not operation.reduces_to_sql:
+            if collect_sql:
                 schema_editor.collected_sql.append("--")
-                schema_editor.collected_sql.append("-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE "
-                                                   "WRITTEN AS SQL:")
+                if not operation.reduces_to_sql:
+                    schema_editor.collected_sql.append(
+                        "-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:"
+                    )
                 schema_editor.collected_sql.append("-- %s" % operation.describe())
                 schema_editor.collected_sql.append("--")
-                continue
+                if not operation.reduces_to_sql:
+                    continue
             # Save the state before the operation has run
             old_state = project_state.clone()
             operation.state_forwards(self.app_label, project_state)
@@ -142,12 +145,14 @@ class Migration(object):
         # Phase 2
         for operation, to_state, from_state in to_run:
             if collect_sql:
+                schema_editor.collected_sql.append("--")
+                if not operation.reduces_to_sql:
+                    schema_editor.collected_sql.append(
+                        "-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE WRITTEN AS SQL:"
+                    )
+                schema_editor.collected_sql.append("-- %s" % operation.describe())
+                schema_editor.collected_sql.append("--")
                 if not operation.reduces_to_sql:
-                    schema_editor.collected_sql.append("--")
-                    schema_editor.collected_sql.append("-- MIGRATION NOW PERFORMS OPERATION THAT CANNOT BE "
-                                                       "WRITTEN AS SQL:")
-                    schema_editor.collected_sql.append("-- %s" % operation.describe())
-                    schema_editor.collected_sql.append("--")
                     continue
             if not schema_editor.connection.features.can_rollback_ddl and operation.atomic:
                 # We're forcing a transaction on a non-transactional-DDL backend

docs/intro/tutorial01.txt

@@ -446,8 +446,8 @@ You should see something similar to the following:
 
     Migrations for 'polls':
       0001_initial.py:
-        - Create model Question
         - Create model Choice
+        - Create model Question
         - Add field question to choice
 
 By running ``makemigrations``, you're telling Django that you've made
@@ -476,16 +476,25 @@ readability):
 .. code-block:: sql
 
     BEGIN;
+    --
+    -- Create model Choice
+    --
     CREATE TABLE "polls_choice" (
         "id" serial NOT NULL PRIMARY KEY,
         "choice_text" varchar(200) NOT NULL,
         "votes" integer NOT NULL
     );
+    --
+    -- Create model Question
+    --
     CREATE TABLE "polls_question" (
         "id" serial NOT NULL PRIMARY KEY,
         "question_text" varchar(200) NOT NULL,
         "pub_date" timestamp with time zone NOT NULL
     );
+    --
+    -- Add field question to choice
+    --
     ALTER TABLE "polls_choice" ADD COLUMN "question_id" integer NOT NULL;
     ALTER TABLE "polls_choice" ALTER COLUMN "question_id" DROP DEFAULT;
     CREATE INDEX "polls_choice_7aa0f6ee" ON "polls_choice" ("question_id");

docs/ref/contrib/gis/tutorial.txt

@@ -286,6 +286,9 @@ This command should produce the following output:
 .. code-block:: sql
 
     BEGIN;
+    --
+    -- Create model WorldBorder
+    --
     CREATE TABLE "world_worldborder" (
         "id" serial NOT NULL PRIMARY KEY,
         "name" varchar(50) NOT NULL,

docs/ref/django-admin.txt

@@ -984,6 +984,12 @@ By default, the SQL created is for running the migration in the forwards
 direction. Pass ``--backwards`` to generate the SQL for
 unapplying the migration instead.
 
+.. versionchanged:: 1.9
+
+    To increase the readability of the overall SQL output the SQL code
+    generated for each migration operation is preceded by the operation's
+    description.
+
 sqlsequencereset <app_label app_label ...>
 ------------------------------------------
 

docs/releases/1.9.txt

@@ -152,6 +152,10 @@ Management Commands
 * The new :djadmin:`sendtestemail` command lets you send a test email to
   easily confirm that email sending through Django is working.
 
+* To increase the readability of the SQL code generated by
+  :djadmin:`sqlmigrate`, the SQL code generated for each migration operation is
+  preceded by the operation's description.
+
 Models
 ^^^^^^
 

tests/migrations/test_commands.py

@@ -289,29 +289,64 @@ class MigrateTests(MigrationTestBase):
         )
 
     @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations"})
-    def test_sqlmigrate(self):
+    def test_sqlmigrate_forwards(self):
         """
         Makes sure that sqlmigrate does something.
         """
-        # Make sure the output is wrapped in a transaction
         out = six.StringIO()
         call_command("sqlmigrate", "migrations", "0001", stdout=out)
-        output = out.getvalue()
-        self.assertIn(connection.ops.start_transaction_sql(), output)
-        self.assertIn(connection.ops.end_transaction_sql(), output)
-
-        # Test forwards. All the databases agree on CREATE TABLE, at least.
-        out = six.StringIO()
-        call_command("sqlmigrate", "migrations", "0001", stdout=out)
-        self.assertIn("create table", out.getvalue().lower())
+        output = out.getvalue().lower()
+
+        index_tx_start = output.find(connection.ops.start_transaction_sql().lower())
+        index_op_desc_author = output.find('-- create model author')
+        index_create_table = output.find('create table')
+        index_op_desc_tribble = output.find('-- create model tribble')
+        index_op_desc_unique_together = output.find('-- alter unique_together')
+        index_tx_end = output.find(connection.ops.end_transaction_sql().lower())
+
+        self.assertGreater(index_tx_start, -1, "Transaction start not found")
+        self.assertGreater(index_op_desc_author, index_tx_start,
+            "Operation description (author) not found or found before transaction start")
+        self.assertGreater(index_create_table, index_op_desc_author,
+            "CREATE TABLE not found or found before operation description (author)")
+        self.assertGreater(index_op_desc_tribble, index_create_table,
+            "Operation description (tribble) not found or found before CREATE TABLE (author)")
+        self.assertGreater(index_op_desc_unique_together, index_op_desc_tribble,
+            "Operation description (unique_together) not found or found before operation description (tribble)")
+        self.assertGreater(index_tx_end, index_op_desc_unique_together,
+            "Transaction end not found or found before operation description (unique_together)")
 
+    @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations"})
+    def test_sqlmigrate_backwards(self):
+        """
+        Makes sure that sqlmigrate does something.
+        """
         # Cannot generate the reverse SQL unless we've applied the migration.
         call_command("migrate", "migrations", verbosity=0)
 
-        # And backwards is a DROP TABLE
         out = six.StringIO()
         call_command("sqlmigrate", "migrations", "0001", stdout=out, backwards=True)
-        self.assertIn("drop table", out.getvalue().lower())
+        output = out.getvalue().lower()
+
+        index_tx_start = output.find(connection.ops.start_transaction_sql().lower())
+        index_op_desc_unique_together = output.find('-- alter unique_together')
+        index_op_desc_tribble = output.find('-- create model tribble')
+        index_op_desc_author = output.find('-- create model author')
+        index_drop_table = output.rfind('drop table')
+        index_tx_end = output.find(connection.ops.end_transaction_sql().lower())
+
+        self.assertGreater(index_tx_start, -1, "Transaction start not found")
+        self.assertGreater(index_op_desc_unique_together, index_tx_start,
+            "Operation description (unique_together) not found or found before transaction start")
+        self.assertGreater(index_op_desc_tribble, index_op_desc_unique_together,
+            "Operation description (tribble) not found or found before operation description (unique_together)")
+        self.assertGreater(index_op_desc_author, index_op_desc_tribble,
+            "Operation description (author) not found or found before operation description (tribble)")
+
+        self.assertGreater(index_drop_table, index_op_desc_author,
+            "DROP TABLE not found or found before operation description (author)")
+        self.assertGreater(index_tx_end, index_op_desc_unique_together,
+            "Transaction end not found or found before DROP TABLE")
 
         # Cleanup by unmigrating everything
         call_command("migrate", "migrations", "zero", verbosity=0)