diff --git a/django_migration_linter/management/commands/lintmigrations.py b/django_migration_linter/management/commands/lintmigrations.py index 27b62f1..9f9eeb9 100644 --- a/django_migration_linter/management/commands/lintmigrations.py +++ b/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"], diff --git a/django_migration_linter/migration_linter.py b/django_migration_linter/migration_linter.py index 95f97dd..72ff1cf 100644 --- a/django_migration_linter/migration_linter.py +++ b/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( - app_label, migration_name, self.exclude_migration_tests - ) + err, ignored_data, warnings = self.analyse_data_migration(app_label, migration_name) 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"]) - if err["table"]: + error_str = "\t{0}".format(err["msg"]) + 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,12 +235,9 @@ class MigrationLinter(object): if MessageType.WARNING.value in self.quiet: return - for function_name, warning_details in warnings.items(): - for warn_detail in warning_details: - warn_str = "\tWARN '{}': {}".format( - function_name, warn_detail["warn_msg"] - ) - print(warn_str) + for warning_details in warnings: + warn_str = "\tWARN {}".format(warning_details["msg"]) + print(warn_str) def print_summary(self): print("*** Summary:") @@ -361,45 +366,54 @@ class MigrationLinter(object): ) ) - def analyse_data_migration( - self, app_label, migration_name, exclude_migration_tests - ): + def analyse_data_migration(self, app_label, migration_name): 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_warnings = self.lint_runpython(operation) + op_errors, op_ignored, 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(runpython): + def lint_runpython(self, runpython): + function_name = runpython.code.__name__ error = [] + ignored = [] warning = [] if not runpython.reversible: - warning.append( - { - "code": "REVERSIBLE_DATA_MIGRATION", - "warn_msg": "RunPython data migration is not reversible", - } - ) + issue = { + "code": "REVERSIBLE_DATA_MIGRATION", + "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( - { - "code": "NAMING_CONVENTION_RUNPYTHON_ARGS", - "warn_msg": "By convention, RunPython names the two arguments: apps, schema_editor", - } - ) + issue = { + "code": "NAMING_CONVENTION_RUNPYTHON_ARGS", + "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 diff --git a/django_migration_linter/sql_analyser/base.py b/django_migration_linter/sql_analyser/base.py index 10c3e94..ef7b4c3 100644 --- a/django_migration_linter/sql_analyser/base.py +++ b/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, diff --git a/django_migration_linter/sql_analyser/sqlite.py b/django_migration_linter/sql_analyser/sqlite.py index 516952f..21503c6 100644 --- a/django_migration_linter/sql_analyser/sqlite.py +++ b/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", }, { diff --git a/tests/functional/test_cache.py b/tests/functional/test_cache.py index e4d3076..4b942f7 100644 --- a/tests/functional/test_cache.py +++ b/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, diff --git a/tests/functional/test_data_migrations.py b/tests/functional/test_data_migrations.py index 4ff7ae2..1cc1662 100644 --- a/tests/functional/test_data_migrations.py +++ b/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, - include_apps=fixtures.DATA_MIGRATIONS, + self.test_project_path, 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)