Merge pull request #3 from HyreAS/recx/backward-compat-ignore-reason

Make IgnoreMigration without reason backcompat

Author: olagjo

src/django_migration_linter/migration_linter.py

@@ -164,12 +164,42 @@ def lint_migration(self, migration: Migration) -> None:
 
         md5hash = self.get_migration_hash(app_label, migration_name)
 
+        # Check if migration has IgnoreMigration operation without a reason
+        has_ignore_op = any(isinstance(o, IgnoreMigration) for o in operations)
+        if has_ignore_op:
+            reason = self.get_ignore_migration_reason(operations)
+            if not reason:
+                # IgnoreMigration without reason - show error and continue linting
+                error = Issue(
+                    code="IGNORE_MIGRATION_MISSING_REASON",
+                    message="IgnoreMigration operation must include a reason. "
+                    "The migration will be linted because no reason was provided. "
+                    "Please add a reason: IgnoreMigration('your reason here')",
+                    table=None,
+                    column=None,
+                )
+                self.print_linting_msg(
+                    app_label,
+                    migration_name,
+                    f"({error.code}) {error.message}",
+                    MessageType.ERROR,
+                )
+                # Don't ignore - continue to lint this migration
+            else:
+                # IgnoreMigration with reason - ignore the migration
+                msg = f"IGNORE - REASON: {reason}"
+                self.print_linting_msg(
+                    app_label, migration_name, msg, MessageType.IGNORE
+                )
+                self.nb_ignored += 1
+                return
+
         if self.should_ignore_migration(
-            app_label, migration_name, operations, is_initial=migration.initial
+            app_label, migration_name, is_initial=migration.initial
         ):
-            reason = self.get_ignore_migration_reason(operations)
-            msg = f"IGNORE - REASON: {reason}" if reason else "IGNORE"
-            self.print_linting_msg(app_label, migration_name, msg, MessageType.IGNORE)
+            self.print_linting_msg(
+                app_label, migration_name, "IGNORE", MessageType.IGNORE
+            )
             self.nb_ignored += 1
             return
 
@@ -479,13 +509,13 @@ def should_ignore_migration(
         self,
         app_label: str,
         migration_name: str,
-        operations: Iterable[Operation] = (),
         is_initial: bool = False,
     ) -> bool:
+        # Note: IgnoreMigration is handled separately in lint_migration()
+        # to check for missing reason
         return (
             (self.include_apps and app_label not in self.include_apps)
             or (self.exclude_apps and app_label in self.exclude_apps)
-            or any(isinstance(o, IgnoreMigration) for o in operations)
             or (
                 self.ignore_name_contains
                 and self.ignore_name_contains in migration_name

src/django_migration_linter/operations.py

@@ -7,27 +7,23 @@ class IgnoreMigration(Operation):
     """
     No-op migration operation that will enable the Django Migration Linter
     to detect if the entire migration should be ignored (through code).
-
-    A reason must be provided to document why this migration is being ignored.
     """
 
     reversible = True
     reduces_to_sql = False
     elidable = True
 
-    def __init__(self, reason: str):
+    def __init__(self, reason: str | None = None):
         """
-        Initialize IgnoreMigration with a required reason.
+        Initialize IgnoreMigration with an optional reason.
 
         Args:
-            reason: A non-empty string explaining why this migration is being ignored.
-
-        Raises:
-            ValueError: If reason is empty or contains only whitespace.
+            reason: Optional string explaining why this migration is being ignored.
+                   For backward compatibility, this parameter is optional, but providing
+                   a reason is strongly encouraged and will be enforced during linting
+                   for new migrations.
         """
-        if not reason or not reason.strip():
-            raise ValueError("IgnoreMigration requires a non-empty reason")
-        self.reason = reason.strip()
+        self.reason = reason.strip() if reason and reason.strip() else None
 
     def state_forwards(self, app_label, state):
         pass
@@ -39,7 +35,9 @@ def database_backwards(self, app_label, schema_editor, from_state, to_state):
         pass
 
     def describe(self):
-        return (
-            f"The Django migration linter will ignore this migration - "
-            f"Reason: {self.reason}"
-        )
+        if self.reason:
+            return (
+                f"The Django migration linter will ignore this migration - "
+                f"Reason: {self.reason}"
+            )
+        return "The Django migration linter will ignore this migration"

tests/unit/test_linter.py

@@ -193,9 +193,24 @@ def test_get_ignore_migration_reason(self):
         extracted_reason = linter.get_ignore_migration_reason(operations)
         self.assertEqual(extracted_reason, reason)
 
-        # Test without IgnoreMigration operation
-        from django.db.migrations.operations import AddField
-
         operations_without_ignore = []
         extracted_reason = linter.get_ignore_migration_reason(operations_without_ignore)
         self.assertIsNone(extracted_reason)
+
+    def test_should_ignore_migration_without_ignore_operation(self):
+        """Test that should_ignore_migration works for non-IgnoreMigration conditions."""
+        # IgnoreMigration operations are handled separately in lint_migration()
+        # so should_ignore_migration doesn't check for them
+
+        # Test with app exclusion
+        linter = MigrationLinter(exclude_apps=["app_excluded"])
+        self.assertTrue(
+            linter.should_ignore_migration(
+                "app_excluded", "0001_test", is_initial=False
+            )
+        )
+        self.assertFalse(
+            linter.should_ignore_migration(
+                "app_included", "0001_test", is_initial=False
+            )
+        )

tests/unit/test_operations.py

@@ -18,19 +18,33 @@ def test_ignore_migration_strips_whitespace(self):
         operation = IgnoreMigration(reason)
         self.assertEqual(operation.reason, "Valid reason with whitespace")
 
-    def test_ignore_migration_without_reason_raises_error(self):
-        """Test that IgnoreMigration raises ValueError when no reason is provided."""
-        with self.assertRaises(TypeError) as _:
-            IgnoreMigration()
-
-    def test_ignore_migration_with_empty_string_raises_error(self):
-        """Test that IgnoreMigration raises ValueError for empty string."""
-        with self.assertRaises(ValueError) as context:
-            IgnoreMigration("")
-        self.assertIn("non-empty reason", str(context.exception))
-
-    def test_ignore_migration_with_whitespace_only_raises_error(self):
-        """Test that IgnoreMigration raises ValueError for whitespace-only string."""
-        with self.assertRaises(ValueError) as context:
-            IgnoreMigration("   ")
-        self.assertIn("non-empty reason", str(context.exception))
+    def test_ignore_migration_without_reason_allowed(self):
+        """Test that IgnoreMigration can be instantiated without a reason for backward compatibility."""
+        operation = IgnoreMigration()
+        self.assertIsNone(operation.reason)
+
+    def test_ignore_migration_with_empty_string(self):
+        """Test that IgnoreMigration with empty string sets reason to None."""
+        operation = IgnoreMigration("")
+        self.assertIsNone(operation.reason)
+
+    def test_ignore_migration_with_whitespace_only(self):
+        """Test that IgnoreMigration with whitespace-only string sets reason to None."""
+        operation = IgnoreMigration("   ")
+        self.assertIsNone(operation.reason)
+
+    def test_ignore_migration_describe_with_reason(self):
+        """Test that describe() includes reason when provided."""
+        reason = "Test reason"
+        operation = IgnoreMigration(reason)
+        description = operation.describe()
+        self.assertIn(reason, description)
+        self.assertIn("Reason:", description)
+
+    def test_ignore_migration_describe_without_reason(self):
+        """Test that describe() handles missing reason gracefully."""
+        operation = IgnoreMigration()
+        description = operation.describe()
+        self.assertEqual(
+            description, "The Django migration linter will ignore this migration"
+        )