Migration Create/Apply Generators #30
Conversation
There was a problem hiding this comment.
Pull Request Overview
Swaps the migration manager’s create_migrations and apply_migrations methods to use async generators, lets the CLI handle printing status, and updates tests to iterate over those generators.
- Change
create_migrationsandapply_migrationsto returnAsyncGeneratorand yield migrations - Move print statements from manager into CLI commands and wrap operations in
try/except - Update end-to-end tests to consume async generators with
async for
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tortoise_pathway/migration_manager.py | Refactored methods to async generators, updated return types and docstrings |
| tortoise_pathway/cli.py | Updated CLI to iterate over migrations, print results, and handle errors |
| tests/e2e/test_no_migrations_app.py | Changed await calls to async for loops |
| tests/e2e/test_model_changes_app.py | Changed await calls to async for loops |
| tests/e2e/test_applied_migrations_app.py | Changed await calls to async for loops |
Comments suppressed due to low confidence (1)
tortoise_pathway/migration_manager.py:113
- The docstring mentions a ValueError for empty migrations with no app specified, but the code never raises this error. Either remove this entry or implement the check as documented.
ValueError: If no app is specified for an empty migration
tortoise_pathway/cli.py
Outdated
| else: | ||
| # Create an empty migration | ||
| migrations = await manager.create_migrations(name, app=app, auto=False) | ||
| # Generate automatic migration based on model changes? |
There was a problem hiding this comment.
[nitpick] The comment ends with a question mark and may be unclear. Consider rephrasing to a descriptive statement (e.g., "Determine whether to auto-generate migrations").
| # Generate automatic migration based on model changes? | |
| # Determine whether to generate automatic migrations based on model changes. |
henadzit
left a comment
henadzit
left a comment
There was a problem hiding this comment.
Looks good! I left a single message about exception reporting.
tortoise_pathway/cli.py
Outdated
| else: | ||
| print("No migrations were applied.") | ||
| except Exception as e: | ||
| print(f"Error applying migrations: {e}") |
There was a problem hiding this comment.
This will provide very limited information about the exception to the user. At this stage, it would be beneficial to have the whole stack trace which users can report, so, I think we either need to improve the exception reporting or not catch the exception at all.
| if reverted: | ||
| print(f"Successfully reverted migration: {reverted.display_name()}") | ||
| else: | ||
| print("No migration was reverted.") |
2 participants
Finally, I can make smaller PRs 😂
This swaps migration manager to using generators and yielding migrations for create/apply. This allows people to customize the output and show results as they occur when implementing the manager in their own tooling. I also moved the responsibility of printing status to the CLI for these functions.