MPT-17670: add --check option to validate duplicate migration_id in migrat…#31
Conversation
📝 WalkthroughWalkthroughAdds a migrations validation flow: a CLI Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mpt_tool/commands/check.py`:
- Line 8: The module/function docstring in check.py currently says "Checks
migrations for duplicate order_id" but the command actually validates duplicate
migration_id; update the docstring text to say "Checks migrations for duplicate
migration_id" (or similar phrasing) so it matches the implemented behavior and
any references to the check command.
🧹 Nitpick comments (6)
docs/PROJECT_DESCRIPTION.md (1)
151-156: Add a language specifier to the fenced code block.The example output code block should have a language specifier (e.g.,
textorconsole) for consistency and proper rendering.📝 Suggested fix
**Example output when duplicates are found:** -``` +```text Checking migrations... Error running check command: Duplicate migration_id found in migrations: 20260113180013_duplicate_name.py, 20260114190014_duplicate_name.py</details> </blockquote></details> <details> <summary>mpt_tool/use_cases/check_migrations.py (2)</summary><blockquote> `14-14`: **Remove unused `noqa` directive.** The `# noqa: WPS210` directive is flagged by Ruff as unknown/unused. WPS codes are from wemake-python-styleguide, which is not in use here. <details> <summary>🧹 Suggested fix</summary> ```diff - def execute(self) -> None: # noqa: WPS210 + def execute(self) -> None:
29-29: Consider using a generator expression instead of a list comprehension.A generator expression is slightly more efficient when passing to
Countersince it avoids creating an intermediate list.♻️ Suggested fix
- migration_id_counter = Counter([migration.migration_id for migration in migration_files]) + migration_id_counter = Counter(migration.migration_id for migration in migration_files)mpt_tool/commands/factory.py (1)
32-32: Remove unusednoqadirective.The
# noqa: WPS242directive is flagged by Ruff as unknown/unused. WPS codes are from wemake-python-styleguide, which is not in use here.🧹 Suggested fix
- match param_data: # noqa: WPS242 + match param_data:tests/cli/test_cli.py (1)
127-132: Use@pytest.mark.usefixturesfor side-effect-only fixtures.The
migration_folderargument is used only for its side effect (creating the folder) but not referenced in the test body. Using the decorator makes the intent clearer and silences the Ruff ARG001 warning.♻️ Suggested fix
+@pytest.mark.usefixtures("migration_folder") -def test_migrate_check_empty_folder(runner, migration_folder): +def test_migrate_check_empty_folder(runner): result = runner.invoke(app, ["migrate", "--check"]) assert result.exit_code == 0, result.output assert "Checking migrations..." in result.output assert "Migrations check passed successfully." in result.outputtests/use_cases/test_check_migrations.py (1)
50-64: Consider verifying all duplicate IDs are reported in the error message.The test creates two duplicate pairs (
duplicate_oneandduplicate_two) but only asserts the generic error message. Strengthening assertions would ensure both duplicate sets are captured.🧪 Suggested fix
with pytest.raises(CheckMigrationError) as exc_info: use_case.execute() assert "Duplicate migration_id found in migrations" in str(exc_info.value) + assert "duplicate_one" in str(exc_info.value) + assert "duplicate_two" in str(exc_info.value)
svazquezco
force-pushed
the
MPT-17670-implement-check-option
branch
from
7ac1860 to
f9b9233
Compare
|
2 participants
…ions
Closes MPT-17670
--checkoption to themigrateCLI to validate duplicatemigration_idvaluesCheckMigrationsUseCaseto detect duplicate migration IDs and raiseCheckMigrationErrorwith affected filenamesCheckCommandCLI command and wire it into the command factory routingCheckMigrationError(and clean up related use-case errors)