Repository: 3YOURMIND/django-migration-linter
Pull request #159 (2021-08-02 23:42:24)

Default ends_with_default to `False`
Problem: Previously in gh-117 we aimed to solve gh-115, which described a problem where we used `SET DEFAULT` and then `DROP DEFAULT` immediately afterward. The solution was to track whether any defaults are retained, but the `ends_with_default` variable is initialized as `None` rather than `False`. At the end of the function we check whether `ends_with_default is False`, never occurs unless a default is set.

Solution: Assume that there is no default unless proven otherwise, and iniitializie `ends_with_default` to `False` instead of `None`.

Fixes: gh-158

christianbundy: @David-Wobrock Is there anything I can do to help move this forward?
codecov-commenter: # [Codecov](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#159](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (c60e90b) into [main](https://codecov.io/gh/3YOURMIND/django-migration-linter/commit/3f0531f349c4c237a5ff1afd594956a58103dc5d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (3f0531f) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159/graphs/tree.svg?width=650&height=150&src=pr&token=pgNNXGbCf7&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

```diff
@@           Coverage Diff           @@
##             main     #159   +/-   ##
=======================================
  Coverage   93.09%   93.09%           
=======================================
  Files          77       77           
  Lines        1637     1637           
=======================================
  Hits         1524     1524           
  Misses        113      113           
```


| [Impacted Files](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [django\_migration\_linter/sql\_analyser/base.py](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvc3FsX2FuYWx5c2VyL2Jhc2UucHk=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Last update [3f0531f...c60e90b](https://codecov.io/gh/3YOURMIND/django-migration-linter/pull/159?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

David-Wobrock: > @David-Wobrock Is there anything I can do to help move this forward?

Nothing in particulier :) I just needed some time to look at the patch.
I took the liberty to push some unit tests additionally, based on your message in the issue.

Thanks a lot for the PR!

Patch:
diff --git a/django_migration_linter/sql_analyser/base.py b/django_migration_linter/sql_analyser/base.py
index 584ebbc..a74b40f 100644
--- a/django_migration_linter/sql_analyser/base.py
+++ b/django_migration_linter/sql_analyser/base.py
@@ -8,7 +8,7 @@ logger = logging.getLogger("django_migration_linter")
 
 def has_not_null_column(sql_statements, **kwargs):
     # TODO: improve to detect that the same column is concerned
-    ends_with_default = None
+    ends_with_default = False
     for sql in sql_statements:
         if "SET DEFAULT" in sql:
             ends_with_default = True


Test patch:
diff --git a/tests/unit/test_sql_analyser.py b/tests/unit/test_sql_analyser.py
index bb30d99..00dd50e 100644
--- a/tests/unit/test_sql_analyser.py
+++ b/tests/unit/test_sql_analyser.py
@@ -171,6 +171,21 @@ class PostgresqlAnalyserTestCase(SqlAnalyserTestCase):
         ]
         self.assertValidSql(sql)
 
+    def test_field_to_not_null_with_dropped_default(self):
+        sql = [
+            'ALTER TABLE "api_example_example" ALTER COLUMN "foo_id" SET DEFAULT 42;',
+            'UPDATE "example_example" SET "foo_id" = 42 WHERE "foo_id" IS NULL;',
+            'ALTER TABLE "example_example" ALTER COLUMN "foo_id" SET NOT NULL;',
+            'ALTER TABLE "example_example" ALTER COLUMN "foo_id" DROP DEFAULT;',
+        ]
+        self.assertBackwardIncompatibleSql(sql)
+
+    def test_onetoonefield_to_not_null(self):
+        sql = [
+            'ALTER TABLE "example_example" ALTER COLUMN "foo" SET NOT NULL;',
+        ]
+        self.assertBackwardIncompatibleSql(sql)
+
     def test_drop_not_null(self):
         sql = 'ALTER TABLE "app_alter_column_drop_not_null_a" ALTER COLUMN "not_null_field" DROP NOT NULL;'
         self.assertValidSql(sql)