Skip to content

Conversation

@LanceMoe
Copy link
Contributor

No description provided.

@LanceMoe
Copy link
Contributor Author

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.

Copy link

@baseplate-admin baseplate-admin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

私のアドバイスを聞いてくれてありがとう

Copy link
Contributor

@waketzheng waketzheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key=lambda x: int(x.split("_")[0]) show that if there is any file whose name does not startswith a number will raise a ValueError.
So this function can be refactor to be:

def get_all_version_files(cls) -> List[str]:
        pattern = re.compile(r"(\d+)_")
        files = [i.name for i in cls.migrate_location.glob("*.py") if pattern.match(i.name)]
        return sorted(files, key=lambda x: int(pattern.search(x).group(1)))

@LanceMoe LanceMoe requested a review from waketzheng December 4, 2024 19:02
@LanceMoe
Copy link
Contributor Author

LanceMoe commented Dec 4, 2024

@waketzheng
Based on your code, I commit a code that can pass mypy. Please review again, thank you.

@waketzheng
Copy link
Contributor

@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:

    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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor

Looks nice, just change to this:

     @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()

@LanceMoe
Copy link
Contributor Author

@waketzheng
Modified and committed, please check again, thank you!

@waketzheng waketzheng merged commit 6270c47 into tortoise:dev Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants