Repository: tortoise/aerich
Pull request #272 (2022-10-17 19:04:40)

fix: error when there is __init__.py in the migration folder

LanceMoe: Aerich changed from `sql` to `py` source code. When there is `__init__.py` in the migration folder, aerich will report an error. This PR fixes the issue.
LanceMoe: @waketzheng 
Based on your code, I commit a code that can pass mypy. Please review again, thank you.
waketzheng: @LanceMoe This PR is useful. It allow user to add custom file like `utils.py` to the migration directory.

Code can be as simple as:
```py
    def get_all_version_files(cls) -> List[str]:
        files = [p.name for p in cls.migrate_location.glob("*.py") if p.name[0].isdigit()]
        return sorted(files, key=lambda x: int(x.split("_")[0]))
```
Note: `import re` should be removed, and would be better if you can add some test cases.
LanceMoe: @waketzheng 
Thanks for your suggestion, but I think this will cause some potential bugs. For example
```
if p.name[0].isdigit()
```
If `p.name` is `'123foo'`, `p.name[0].isdigit()` will be true, but `int(x.split("_")[0])` will raise an error.
I'll see how to improve it.
LanceMoe: @waketzheng 
Following your idea, I submitted another change and added tests.

Finally, I chose to add two closures in this function for the following reasons:
1. I think that although it can be combined into one line, but it will make the rules of version files unclear. Through the `is_version_file` function, we can clearly know what rules a migration file should follow: It is a `.py` file, and the file name contains an underscore, and the underscore in the file name is preceded by a number.
2. Although `split("_")[0]` is not long, it is reused. Wrapping it into a function can not only improve the readability of the code, but also prevent mistakes during refactoring of feature changes in the future. Although this function has only one line, the reason why I did not use lambda to define it, is that it is not convenient to add type hints when lambda is defined alone. This requires importing `Callable` from the `typing` library. I think this loses the meaning of using lambda. It is better to define it as a normal closure function.
3. Although the type of `migrate_location` is marked as a `Path`, the existing test code mocks `os.listdir`, and `migrate_location` is directly passed a `str` value, so it becomes inconvenient to use `glob("*.py")` I chose to switch back to `filter`+`os.listdir` so that I don't have to modify the test code a lot. If refactoring is needed, I think it should be opened as a separate task instead of solving it in this PR.

Please review again, thank you.
waketzheng: Looks nice, just change to this:
```diff
     @classmethod
     def get_all_version_files(cls) -> List[str]:
-        def get_file_version(file_name: str):
+        def get_file_version(file_name: str) -> str:
             return file_name.split("_")[0]

-        def is_version_file(file_name: str):
+        def is_version_file(file_name: str) -> bool:
             if not file_name.endswith("py"):
                 return False
-            if file_name.find("_") <= 0:
+            if "_" not in file_name:
                 return False
             return get_file_version(file_name).isdigit()
```

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f518021..094dc16 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,7 @@
 ### [0.8.1](Unreleased)
 
 #### Fixed
+- fix: error when there is __init__.py in the migration folder (#272)
 - Setting null=false on m2m field causes migration to fail. (#334)
 - Fix NonExistentKey when running `aerich init` without `[tool]` section in config file. (#284)
 - Fix configuration file reading error when containing Chinese characters. (#286)
diff --git a/aerich/migrate.py b/aerich/migrate.py
index 5429ac8..3158b93 100644
--- a/aerich/migrate.py
+++ b/aerich/migrate.py
@@ -54,10 +54,18 @@ class Migrate:
 
     @classmethod
     def get_all_version_files(cls) -> List[str]:
-        return sorted(
-            filter(lambda x: x.endswith("py"), os.listdir(cls.migrate_location)),
-            key=lambda x: int(x.split("_")[0]),
-        )
+        def get_file_version(file_name: str) -> str:
+            return file_name.split("_")[0]
+
+        def is_version_file(file_name: str) -> bool:
+            if not file_name.endswith("py"):
+                return False
+            if "_" not in file_name:
+                return False
+            return get_file_version(file_name).isdigit()
+
+        files = filter(is_version_file, os.listdir(cls.migrate_location))
+        return sorted(files, key=lambda x: int(get_file_version(x)))
 
     @classmethod
     def _get_model(cls, model: str) -> Type[Model]:


Test patch:
diff --git a/tests/test_migrate.py b/tests/test_migrate.py
index 1f7802a..6cb1d33 100644
--- a/tests/test_migrate.py
+++ b/tests/test_migrate.py
@@ -1054,6 +1054,30 @@ def test_sort_all_version_files(mocker):
     ]
 
 
+def test_sort_files_containing_non_migrations(mocker):
+    mocker.patch(
+        "os.listdir",
+        return_value=[
+            "1_datetime_update.py",
+            "11_datetime_update.py",
+            "10_datetime_update.py",
+            "2_datetime_update.py",
+            "not_a_migration.py",
+            "999.py",
+            "123foo_not_a_migration.py",
+        ],
+    )
+
+    Migrate.migrate_location = "."
+
+    assert Migrate.get_all_version_files() == [
+        "1_datetime_update.py",
+        "2_datetime_update.py",
+        "10_datetime_update.py",
+        "11_datetime_update.py",
+    ]
+
+
 async def test_empty_migration(mocker, tmp_path: Path) -> None:
     mocker.patch("os.listdir", return_value=[])
     Migrate.app = "foo"