Handle warning as error and exclude warning tests

django_migration_linter/management/commands/lintmigrations.py

@@ -109,6 +109,9 @@ class Command(BaseCommand):
             choices=MessageType.values(),
             help="don't print linting messages to stdout",
         )
+        parser.add_argument(
+            "--warning-as-error", action="store_true", help="handle warnings as errors",
+        )
 
     def handle(self, *args, **options):
         if options["project_root_path"]:
@@ -136,6 +139,7 @@ class Command(BaseCommand):
             only_unapplied_migrations=options["unapplied_migrations"],
             exclude_migration_tests=options["exclude_migration_tests"],
             quiet=options["quiet"],
+            warning_as_error=options["warning_as_error"],
         )
         linter.lint_all_migrations(
             git_commit_id=options["commit_id"],

django_migration_linter/migration_linter.py

@@ -52,6 +52,7 @@ class MigrationLinter(object):
         only_unapplied_migrations=False,
         exclude_migration_tests=None,
         quiet=None,
+        warning_as_error=False,
     ):
         # Store parameters and options
         self.django_path = path
@@ -66,6 +67,7 @@ class MigrationLinter(object):
         self.only_applied_migrations = only_applied_migrations
         self.only_unapplied_migrations = only_unapplied_migrations
         self.quiet = quiet or []
+        self.warning_as_error = warning_as_error
 
         # Initialise counters
         self.nb_valid = 0
@@ -134,12 +136,17 @@ class MigrationLinter(object):
             self.exclude_migration_tests,
         )
 
-        err, warnings = self.analyse_data_migration(
+        err, ignored_data, warnings = self.analyse_data_migration(app_label, migration_name)
-            app_label, migration_name, self.exclude_migration_tests
-        )
         if err:
             errors += err
+        if ignored_data:
+            ignored += ignored_data
 
+        if self.warning_as_error:
+            errors += warnings
+            warnings = []
+
+        # Fixme: have a more generic approach to handling errors/warnings/ignored/ok?
         if errors:
             self.print_linting_msg(app_label, migration_name, "ERR", MessageType.ERROR)
             self.nb_erroneous += 1
@@ -154,6 +161,7 @@ class MigrationLinter(object):
             self.nb_warnings += 1
             self.print_warnings(warnings)
             value_to_cache = {"result": "WARNING", "warnings": warnings}
+            # Fixme: not displaying ignored errors, when
         else:
             if ignored:
                 self.print_linting_msg(
@@ -215,10 +223,10 @@ class MigrationLinter(object):
         if MessageType.ERROR.value in self.quiet:
             return
         for err in errors:
-            error_str = "\t{0}".format(err["err_msg"])
+            error_str = "\t{0}".format(err["msg"])
-            if err["table"]:
+            if err.get("table"):
                 error_str += " (table: {0}".format(err["table"])
-                if err["column"]:
+                if err.get("column"):
                     error_str += ", column: {0}".format(err["column"])
                 error_str += ")"
             print(error_str)
@@ -227,11 +235,8 @@ class MigrationLinter(object):
         if MessageType.WARNING.value in self.quiet:
             return
 
-        for function_name, warning_details in warnings.items():
+        for warning_details in warnings:
-            for warn_detail in warning_details:
+            warn_str = "\tWARN {}".format(warning_details["msg"])
-                warn_str = "\tWARN '{}': {}".format(
-                    function_name, warn_detail["warn_msg"]
-                )
             print(warn_str)
 
     def print_summary(self):
@@ -361,45 +366,54 @@ class MigrationLinter(object):
             )
         )
 
-    def analyse_data_migration(
+    def analyse_data_migration(self, app_label, migration_name):
-        self, app_label, migration_name, exclude_migration_tests
-    ):
         migration = self.migration_loader.disk_migrations[(app_label, migration_name)]
         errors = []
-        warnings = defaultdict(list)
+        ignored = []
+        warnings = []
 
         for operation in migration.operations:
             if isinstance(operation, migrations.RunPython):
-                function_name = operation.code.__name__
+                op_errors, op_ignored, op_warnings = self.lint_runpython(operation)
-                op_errors, op_warnings = self.lint_runpython(operation)
                 if op_errors:
                     errors += op_errors
+                if op_ignored:
+                    ignored += op_ignored
                 if op_warnings:
-                    warnings[function_name] = op_warnings
+                    warnings += op_warnings
 
-        return errors, warnings
+        return errors, ignored, warnings
 
-    @staticmethod
+    def lint_runpython(self, runpython):
-    def lint_runpython(runpython):
+        function_name = runpython.code.__name__
         error = []
+        ignored = []
         warning = []
 
         if not runpython.reversible:
-            warning.append(
+            issue = {
-                {
                 "code": "REVERSIBLE_DATA_MIGRATION",
-                    "warn_msg": "RunPython data migration is not reversible",
+                "msg": "'{}': RunPython data migration is not reversible".format(
+                    function_name
+                ),
             }
-            )
+            if issue["code"] in self.exclude_migration_tests:
+                ignored.append(issue)
+            else:
+                warning.append(issue)
 
         args_spec = inspect.getfullargspec(runpython.code)
         if tuple(args_spec.args) != EXPECTED_DATA_MIGRATION_ARGS:
-            warning.append(
+            issue = {
-                {
                 "code": "NAMING_CONVENTION_RUNPYTHON_ARGS",
-                    "warn_msg": "By convention, RunPython names the two arguments: apps, schema_editor",
+                "msg": "'{}': By convention, RunPython names the two arguments: apps, schema_editor".format(
+                    function_name
+                ),
             }
-            )
+            if issue["code"] in self.exclude_migration_tests:
+                ignored.append(issue)
+            else:
+                warning.append(issue)
 
         # TODO: test if importing model directly or correctly using "get_model()"
-        return error, warning
+        return error, ignored, warning

django_migration_linter/sql_analyser/base.py

@@ -37,32 +37,32 @@ class BaseAnalyser(object):
             "code": "RENAME_TABLE",
             "fn": lambda sql, **kw: re.search("RENAME TABLE", sql)
             or re.search("ALTER TABLE .* RENAME TO", sql),
-            "err_msg": "RENAMING tables",
+            "msg": "RENAMING tables",
             "mode": "one_liner",
         },
         {
             "code": "NOT_NULL",
             "fn": has_not_null_column,
-            "err_msg": "NOT NULL constraint on columns",
+            "msg": "NOT NULL constraint on columns",
             "mode": "transaction",
         },
         {
             "code": "DROP_COLUMN",
             "fn": lambda sql, **kw: re.search("DROP COLUMN", sql),
-            "err_msg": "DROPPING columns",
+            "msg": "DROPPING columns",
             "mode": "one_liner",
         },
         {
             "code": "DROP_TABLE",
             "fn": lambda sql, **kw: sql.startswith("DROP TABLE"),
-            "err_msg": "DROPPING table",
+            "msg": "DROPPING table",
             "mode": "one_liner",
         },
         {
             "code": "RENAME_COLUMN",
             "fn": lambda sql, **kw: re.search("ALTER TABLE .* CHANGE", sql)
             or re.search("ALTER TABLE .* RENAME COLUMN", sql),
-            "err_msg": "RENAMING columns",
+            "msg": "RENAMING columns",
             "mode": "one_liner",
         },
         {
@@ -70,7 +70,7 @@ class BaseAnalyser(object):
             "fn": lambda sql, **kw: re.search(
                 "ALTER TABLE .* ALTER COLUMN .* TYPE", sql
             ),
-            "err_msg": (
+            "msg": (
                 "ALTERING columns (Could be backward compatible. "
                 "You may ignore this migration.)"
             ),
@@ -79,7 +79,7 @@ class BaseAnalyser(object):
         {
             "code": "ADD_UNIQUE",
             "fn": has_add_unique,
-            "err_msg": "ADDING unique constraint",
+            "msg": "ADDING unique constraint",
             "mode": "transaction",
         },
     ]
@@ -126,7 +126,7 @@ class BaseAnalyser(object):
         table = self.detect_table(sql_statement)
         col = self.detect_column(sql_statement)
         return {
-            "err_msg": migration_test["err_msg"],
+            "msg": migration_test["msg"],
             "code": migration_test["code"],
             "table": table,
             "column": col,

django_migration_linter/sql_analyser/sqlite.py

@@ -35,7 +35,7 @@ class SqliteAnalyser(BaseAnalyser):
                 sql.startswith("DROP TABLE") for sql in sql_statements
             )
             and not any(sql.startswith("CREATE TABLE") for sql in sql_statements),
-            "err_msg": "DROPPING table",
+            "msg": "DROPPING table",
             "mode": "transaction",
         },
         {

tests/functional/test_cache.py

@@ -53,7 +53,7 @@ class CacheTestCase(unittest.TestCase):
         self.assertListEqual(
             [
                 {
-                    "err_msg": "NOT NULL constraint on columns",
+                    "msg": "NOT NULL constraint on columns",
                     "code": "NOT_NULL",
                     "table": None,
                     "column": None,
@@ -105,7 +105,7 @@ class CacheTestCase(unittest.TestCase):
         self.assertListEqual(
             [
                 {
-                    "err_msg": "NOT NULL constraint on columns",
+                    "msg": "NOT NULL constraint on columns",
                     "code": "NOT_NULL",
                     "table": None,
                     "column": None,
@@ -132,7 +132,7 @@ class CacheTestCase(unittest.TestCase):
         self.assertListEqual(
             [
                 {
-                    "err_msg": "NOT NULL constraint on columns",
+                    "msg": "NOT NULL constraint on columns",
                     "code": "NOT_NULL",
                     "table": None,
                     "column": None,
@@ -245,7 +245,7 @@ class CacheTestCase(unittest.TestCase):
         self.assertListEqual(
             [
                 {
-                    "err_msg": "NOT NULL constraint on columns",
+                    "msg": "NOT NULL constraint on columns",
                     "code": "NOT_NULL",
                     "table": None,
                     "column": None,

tests/functional/test_data_migrations.py

@@ -11,8 +11,7 @@ class DataMigrationDetectionTestCase(unittest.TestCase):
     def setUp(self, *args, **kwargs):
         self.test_project_path = os.path.dirname(settings.BASE_DIR)
         self.linter = MigrationLinter(
-            self.test_project_path,
+            self.test_project_path, include_apps=fixtures.DATA_MIGRATIONS,
-            include_apps=fixtures.DATA_MIGRATIONS,
         )
 
     def test_reverse_data_migration(self):
@@ -26,7 +25,6 @@ class DataMigrationDetectionTestCase(unittest.TestCase):
         self.assertFalse(self.linter.has_errors)
 
     def test_reverse_data_migration_ignore(self):
-        self.assertEqual(0, self.linter.nb_warnings)
         reverse_migration = self.linter.migration_loader.disk_migrations[
             ("app_data_migrations", "0003_incorrect_arguments")
         ]
@@ -34,3 +32,35 @@ class DataMigrationDetectionTestCase(unittest.TestCase):
 
         self.assertEqual(1, self.linter.nb_warnings)
         self.assertFalse(self.linter.has_errors)
+
+    def test_exclude_warning_from_test(self):
+        self.linter = MigrationLinter(
+            self.test_project_path,
+            include_apps=fixtures.DATA_MIGRATIONS,
+            exclude_migration_tests=("REVERSIBLE_DATA_MIGRATION",),
+        )
+
+        reverse_migration = self.linter.migration_loader.disk_migrations[
+            ("app_data_migrations", "0002_missing_reverse")
+        ]
+        self.linter.lint_migration(reverse_migration)
+
+        self.assertEqual(0, self.linter.nb_warnings)
+        self.assertEqual(1, self.linter.nb_valid)
+        self.assertFalse(self.linter.has_errors)
+
+    def test_warning_as_error(self):
+        self.linter = MigrationLinter(
+            self.test_project_path,
+            include_apps=fixtures.DATA_MIGRATIONS,
+            warning_as_error=True,
+        )
+
+        reverse_migration = self.linter.migration_loader.disk_migrations[
+            ("app_data_migrations", "0003_incorrect_arguments")
+        ]
+        self.linter.lint_migration(reverse_migration)
+
+        self.assertEqual(0, self.linter.nb_warnings)
+        self.assertEqual(1, self.linter.nb_erroneous)
+        self.assertTrue(self.linter.has_errors)