Skip to content

MPT-16844: implement NewMigrationUseCase#16

Merged
d3rky merged 1 commit intomainfrom
MPT-16844-implement-new-migration-use-case
Jan 16, 2026
Merged

MPT-16844: implement NewMigrationUseCase#16
d3rky merged 1 commit intomainfrom
MPT-16844-implement-new-migration-use-case

Conversation

@svazquezco
Copy link
Collaborator

@svazquezco svazquezco commented Jan 15, 2026

Closes MPT-16844

  • Implemented NewMigrationUseCase to encapsulate creation of new migration files and replace ad-hoc CLI scaffolding
  • Added CreateMigrationError and NewMigrationError exception classes for clearer error mapping
  • Added FileMigrationManager.new_migration(...) to create migration files, ensure folders, validate IDs, prevent collisions, and write scaffolding templates
  • Enhanced MigrationFile with:
    • file_name property formatted as "<order_id>_<migration_id>.py"
    • post_init validation to ensure migration IDs are valid identifiers
    • new() classmethod to generate timestamped migration filenames and order IDs
  • Refactored CLI migrate command to delegate scaffolding to NewMigrationUseCase (handles NewMigrationError) and removed direct filesystem/template logic
  • Updated migrate CLI options to --new-data and --new-schema (string filename values, mutually exclusive with --data/--schema) and kept run-migration behavior for --data/--schema

@svazquezco svazquezco requested a review from a team as a code owner January 15, 2026 16:50
@svazquezco svazquezco requested review from jentyk and ruben-sebrango and removed request for a team January 15, 2026 16:50
@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

Replaces CLI’s inline migration-file scaffolding with a NewMigrationUseCase that delegates creation to FileMigrationManager.new_migration; adds CreateMigrationError and NewMigrationError, MigrationFile validation and factory methods, and new CLI options --new-data / --new-schema that accept filenames and invoke the use case.

Changes

Cohort / File(s) Summary
Errors
mpt_tool/errors.py
Added CreateMigrationError(BaseError) and NewMigrationError(BaseError) for migration creation failures.
Models
mpt_tool/models.py
MigrationFile: added file_name property, __post_init__ validation enforcing migration_id.isidentifier(), and @classmethod new() that builds timestamped migration filenames.
Managers
mpt_tool/managers.py
Added FileMigrationManager.new_migration(file_suffix: str, migration_type: MigrationTypeEnum) -> MigrationFile: ensures folder, validates IDs, writes scaffolding using MIGRATION_SCAFFOLDING_TEMPLATE, and raises CreateMigrationError on failure. Also tightened validate() return typing and error conversion.
Use Case
mpt_tool/use_cases/new_migration.py
New NewMigrationUseCase with execute(migration_type, file_name) delegating to FileMigrationManager.new_migration and mapping CreateMigrationErrorNewMigrationError, returning the created filename.
CLI
mpt_tool/cli.py
migrate() signature now accepts `new_data: str

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Test Coverage Required ⚠️ Warning PR modifies 5 code files but lacks unit tests for new NewMigrationUseCase, FileMigrationManager.new_migration(), MigrationFile validation, and error classes. Add unit tests covering NewMigrationUseCase.execute(), FileMigrationManager.new_migration(), MigrationFile.new() classmethod, and exception classes in a new test file.
✅ Passed checks (2 passed)
Check name Status Explanation
Jira Issue Key In Title ✅ Passed The PR title contains exactly one Jira issue key MPT-16844 in the correct format, properly positioned at the beginning.
Single Commit Required ✅ Passed The PR contains exactly one commit (hash: da214a3, message: 'refactor: add NewMigrationUseCase'), which satisfies the requirement of keeping git history clean.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


🧹 Recent nitpick comments
mpt_tool/managers.py (1)

27-27: Consider removing unused noqa directive.

Ruff reports WPS238 as unknown (it's a wemake-python-styleguide rule). If the project no longer uses wemake-python-styleguide, this directive can be removed.

mpt_tool/use_cases/new_migration.py (1)

9-10: Consider using the class directly instead of instantiating.

FileMigrationManager uses @classmethod for all its methods, so instantiation isn't necessary. Consider accepting the class type directly or calling FileMigrationManager.new_migration(...) without instantiation.

However, the current approach still works since instance method calls on classmethods are valid in Python.

♻️ Alternative approach
-    def __init__(self, file_manager: FileMigrationManager | None = None):
-        self.file_manager = file_manager or FileMigrationManager()
+    def __init__(self, file_manager: type[FileMigrationManager] | None = None):
+        self.file_manager = file_manager or FileMigrationManager

Then in execute():

migration_file = self.file_manager.new_migration(...)
mpt_tool/cli.py (2)

20-20: Consider removing unused noqa directive.

Ruff reports that C901 is non-enabled and WPS238, WPS231 are unknown (wemake-python-styleguide rules). If these linters are not in use, the directive can be cleaned up.


56-56: Suppress exception chaining with raise typer.Abort from None.

Both raise typer.Abort statements (lines 56 and 69) are within except blocks. Since Typer only prints "Aborted!" without showing the exception chain, and the error message is already communicated via typer.secho(), using from None improves UX by suppressing the internal chain. Based on learnings from this repository.

♻️ Proposed fix
@@ -54,7 +54,7 @@
         except RunMigrationError as error:
             typer.secho(f"Error running migrations: {error!s}", fg=typer.colors.RED)
-            raise typer.Abort
+            raise typer.Abort from None
@@ -67,7 +67,7 @@
         except NewMigrationError as error:
             typer.secho(f"Error creating migration: {error!s}", fg=typer.colors.RED)
-            raise typer.Abort
+            raise typer.Abort from None

Also applies to: 69-69


📜 Recent review details

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f94281a and da214a3.

📒 Files selected for processing (5)
  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/managers.py
  • mpt_tool/models.py
  • mpt_tool/use_cases/new_migration.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: All configuration must be provided via environment variables; do not hardcode configuration values
Use type annotations (PEP 484) in Python files, except in the tests/ folder
All public functions, methods, and classes must include Google-style docstrings
Do not add inline comments; rely on clear code and docstrings instead
Function and variable names must be explicit and intention-revealing
Follow PEP 8 unless explicitly overridden by ruff

Files:

  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py

⚙️ CodeRabbit configuration file

**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.

Files:

  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py
**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py
🧠 Learnings (2)
📚 Learning: 2026-01-15T13:30:45.197Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:30:45.197Z
Learning: In except blocks that raise typer.Abort, raise typer.Abort from None to suppress exception chaining (B904). Since Typer only prints 'Aborted!' and exits, ensure user-facing communication is done before raising Abort (e.g., via typer.secho) and apply this pattern in mpt_tool/cli.py to keep user messages clear.

Applied to files:

  • mpt_tool/cli.py
📚 Learning: 2026-01-15T13:54:01.365Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:54:01.365Z
Learning: When reviewing Ruff lint violations, first check the project's pyproject.toml under [tool.ruff.lint].ignore to see which rules are explicitly ignored. Do not report violations for rules listed in that ignore list. Apply this guidance across all repositories in the softwareone-platform organization that use the same Ruff configuration.

Applied to files:

  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py
🧬 Code graph analysis (3)
mpt_tool/cli.py (4)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/errors.py (1)
  • NewMigrationError (20-21)
mpt_tool/managers.py (1)
  • new_migration (75-98)
mpt_tool/use_cases/new_migration.py (2)
  • NewMigrationUseCase (6-21)
  • execute (12-21)
mpt_tool/use_cases/new_migration.py (4)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/errors.py (2)
  • CreateMigrationError (12-13)
  • NewMigrationError (20-21)
mpt_tool/managers.py (2)
  • FileMigrationManager (21-98)
  • new_migration (75-98)
mpt_tool/models.py (1)
  • file_name (67-69)
mpt_tool/models.py (1)
mpt_tool/managers.py (1)
  • new (138-148)
🪛 Ruff (0.14.11)
mpt_tool/cli.py

20-20: Unused noqa directive (non-enabled: C901; unknown: WPS238, WPS231)

Remove unused noqa directive

(RUF100)

mpt_tool/models.py

73-75: Avoid specifying long messages outside the exception class

(TRY003)

mpt_tool/managers.py

27-27: Unused noqa directive (unknown: WPS238)

Remove unused noqa directive

(RUF100)


30-30: Avoid specifying long messages outside the exception class

(TRY003)


81-81: Avoid specifying long messages outside the exception class

(TRY003)


86-88: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (11)
mpt_tool/errors.py (1)

12-22: LGTM!

The new exception classes CreateMigrationError and NewMigrationError follow the established pattern, inheriting from BaseError with appropriate docstrings. The separation between creation-level errors (manager layer) and use-case-level errors enables proper error translation across layers.

mpt_tool/models.py (3)

66-69: LGTM!

The file_name property correctly formats the migration filename with the .py extension. This complements the existing name property which returns the stem without extension.


71-75: LGTM!

The __post_init__ validation ensures migration IDs are valid Python identifiers, which is important since these become part of file names and module names. The ValueError is properly caught and wrapped at the manager layer.


92-102: LGTM!

The new() factory method correctly generates timestamp-based filenames using UTC for consistent ordering. The __post_init__ validation will automatically run when the instance is created, ensuring invalid migration IDs are caught early.

mpt_tool/managers.py (3)

11-18: LGTM!

Import updates properly bring in CreateMigrationError for the new migration creation flow and MIGRATION_SCAFFOLDING_TEMPLATE for file scaffolding.


32-54: LGTM!

The validate() method now properly catches ValueError from MigrationFile.build_from_path() and wraps it as MigrationFolderError, ensuring consistent error handling for invalid migration filenames. The explicit tuple() return aligns with the type annotation.


74-98: LGTM!

The new_migration() method is well-structured:

  • Creates the migration folder if needed
  • Validates the migration ID via MigrationFile.new() with proper error translation
  • Prevents overwriting existing files with touch(exist_ok=False)
  • Writes the appropriate scaffolding template based on migration type

The unhandled OSError from write_text() was acknowledged in a previous review and deferred for later.

mpt_tool/use_cases/new_migration.py (1)

12-21: LGTM!

The execute() method correctly delegates to FileMigrationManager.new_migration() and properly translates CreateMigrationError to NewMigrationError, maintaining clean separation between the manager and use-case layers.

mpt_tool/cli.py (3)

2-9: LGTM!

Import updates are clean. The cast import is used for type narrowing, and the new error and use case imports support the migration scaffolding feature.


23-38: LGTM!

The new --new-data and --new-schema options are well-defined with appropriate type annotations, metavar hints, and help text. The str | None typing properly reflects the optional nature of these parameters.


61-71: LGTM!

The migration scaffolding logic correctly:

  • Determines migration type based on which option was provided
  • Delegates to NewMigrationUseCase.execute()
  • Handles NewMigrationError with appropriate user feedback
  • Uses cast() safely since the control flow guarantees filename_suffix is not None

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@svazquezco svazquezco marked this pull request as draft January 15, 2026 16:50
@svazquezco
Copy link
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@mpt_tool/models.py`:
- Around line 71-75: In __post_init__, update the ValueError message raised for
migration_id validation to correct the grammar from "must contains only
alphanumeric letters and numbers, or underscores." to "must contain only
alphanumeric characters or underscores." — edit the error string in the
migration_id validation within the __post_init__ method.
🧹 Nitpick comments (2)
mpt_tool/use_cases/new_migration.py (1)

9-10: Consider consistency with RunMigrationsUseCase constructor pattern.

This constructor instantiates FileMigrationManager() when no manager is provided, while RunMigrationsUseCase stores the class reference (FileMigrationManager) without instantiation. Since FileMigrationManager uses only @classmethod methods, both approaches work, but the inconsistency may cause confusion.

Option to align with RunMigrationsUseCase pattern
     def __init__(self, file_manager: FileMigrationManager | None = None):
-        self.file_manager = file_manager or FileMigrationManager()
+        self.file_manager = file_manager or FileMigrationManager

Alternatively, consider refactoring both use cases to use a consistent pattern (either both use class references or both use instances).

mpt_tool/cli.py (1)

65-69: Use raise typer.Abort from None to suppress exception chaining.

Since Typer only prints "Aborted!" on typer.Abort, the user-facing error message is already communicated via typer.secho. Using from None suppresses the exception chain and keeps the output clean.

Based on learnings, this pattern should be applied here. The same applies to line 56 for consistency.

♻️ Proposed fix
         try:
             filename = NewMigrationUseCase().execute(migration_type, cast(str, filename_suffix))
         except NewMigrationError as error:
             typer.secho(f"Error creating migration: {error!s}", fg=typer.colors.RED)
-            raise typer.Abort
+            raise typer.Abort from None

For consistency, also consider updating line 56:

         except RunMigrationError as error:
             typer.secho(f"Error running migrations: {error!s}", fg=typer.colors.RED)
-            raise typer.Abort
+            raise typer.Abort from None
📜 Review details

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84bf46c and 37b53ac.

📒 Files selected for processing (6)
  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/managers.py
  • mpt_tool/models.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/use_cases/run_migrations.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: All configuration must be provided via environment variables; do not hardcode configuration values
Use type annotations (PEP 484) in Python files, except in the tests/ folder
All public functions, methods, and classes must include Google-style docstrings
Do not add inline comments; rely on clear code and docstrings instead
Function and variable names must be explicit and intention-revealing
Follow PEP 8 unless explicitly overridden by ruff

Files:

  • mpt_tool/use_cases/run_migrations.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py
  • mpt_tool/errors.py
  • mpt_tool/cli.py

⚙️ CodeRabbit configuration file

**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.

Files:

  • mpt_tool/use_cases/run_migrations.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py
  • mpt_tool/errors.py
  • mpt_tool/cli.py
**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

  • mpt_tool/use_cases/run_migrations.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py
  • mpt_tool/errors.py
  • mpt_tool/cli.py
🧠 Learnings (4)
📚 Learning: 2026-01-09T16:49:46.680Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 10
File: tests/commands/test_base.py:19-24
Timestamp: 2026-01-09T16:49:46.680Z
Learning: In tests accessing protected attributes like `_type` from command classes, use `# noqa: WPS437` to suppress wemake-python-styleguide's protected attribute usage violation. Ruff may flag this as unused (RUF100) because it doesn't recognize wemake rules, but the noqa comment must be kept for flake8/wemake compliance.

Applied to files:

  • mpt_tool/use_cases/run_migrations.py
📚 Learning: 2026-01-15T13:54:01.365Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:54:01.365Z
Learning: When reviewing Ruff lint violations, first check the project's pyproject.toml under [tool.ruff.lint].ignore to see which rules are explicitly ignored. Do not report violations for rules listed in that ignore list. Apply this guidance across all repositories in the softwareone-platform organization that use the same Ruff configuration.

Applied to files:

  • mpt_tool/use_cases/run_migrations.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/managers.py
  • mpt_tool/errors.py
  • mpt_tool/cli.py
📚 Learning: 2026-01-15T15:01:25.807Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/use_cases/run_migrations.py:58-70
Timestamp: 2026-01-15T15:01:25.807Z
Learning: In mpt_tool/use_cases/run_migrations.py, do not persist a 'started' state before running a migration. If the process crashes or is terminated during migration execution, the migration should be re-attempted from scratch on the next run instead of being treated as started. This ensures crash-recovery and idempotent migrations. Consider adding tests to verify that a partially-run migration does not get stuck and that subsequent runs re-attempt from the beginning. If broader applicability is intended, this behavior should be generalized to the entire migration runner logic.

Applied to files:

  • mpt_tool/use_cases/run_migrations.py
📚 Learning: 2026-01-15T13:30:45.197Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:30:45.197Z
Learning: In except blocks that raise typer.Abort, raise typer.Abort from None to suppress exception chaining (B904). Since Typer only prints 'Aborted!' and exits, ensure user-facing communication is done before raising Abort (e.g., via typer.secho) and apply this pattern in mpt_tool/cli.py to keep user messages clear.

Applied to files:

  • mpt_tool/cli.py
🧬 Code graph analysis (4)
mpt_tool/use_cases/run_migrations.py (2)
mpt_tool/use_cases/new_migration.py (1)
  • execute (12-21)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/use_cases/new_migration.py (4)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/errors.py (2)
  • CreateMigrationError (12-13)
  • NewMigrationError (20-21)
mpt_tool/managers.py (1)
  • new_migration (73-96)
mpt_tool/models.py (1)
  • file_name (67-69)
mpt_tool/managers.py (3)
mpt_tool/errors.py (1)
  • CreateMigrationError (12-13)
mpt_tool/models.py (4)
  • Migration (10-55)
  • MigrationFile (59-102)
  • new (93-102)
  • file_name (67-69)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/cli.py (4)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/use_cases/run_migrations.py (2)
  • RunMigrationsUseCase (16-80)
  • execute (27-70)
mpt_tool/managers.py (1)
  • new_migration (73-96)
mpt_tool/use_cases/new_migration.py (2)
  • NewMigrationUseCase (6-21)
  • execute (12-21)
🪛 Ruff (0.14.11)
mpt_tool/use_cases/run_migrations.py

27-27: Unused noqa directive (non-enabled: C901; unknown: WPS231)

Remove unused noqa directive

(RUF100)

mpt_tool/models.py

73-75: Avoid specifying long messages outside the exception class

(TRY003)

mpt_tool/managers.py

79-79: Avoid specifying long messages outside the exception class

(TRY003)


84-86: Avoid specifying long messages outside the exception class

(TRY003)

mpt_tool/cli.py

20-20: Unused noqa directive (non-enabled: C901; unknown: WPS238, WPS231)

Remove unused noqa directive

(RUF100)

🔇 Additional comments (9)
mpt_tool/use_cases/run_migrations.py (1)

27-27: LGTM - noqa directive is appropriate for wemake-python-styleguide compliance.

The static analysis hint about unused noqa is a false positive. Based on learnings, Ruff doesn't recognize WPS rules from wemake-python-styleguide, but these noqa comments must be retained for flake8/wemake compliance. The removal of WPS213 while keeping C901 and WPS231 appears intentional.

mpt_tool/models.py (2)

66-69: LGTM!

The file_name property correctly formats the migration filename with the .py extension, complementing the existing name property used for module identification.


92-102: LGTM!

The new classmethod properly generates timestamp-based order IDs using UTC time, ensuring consistent ordering across different timezones. The validation of migration_id will be handled by __post_init__ when the instance is created.

mpt_tool/errors.py (1)

12-22: LGTM!

The new error classes follow the established pattern and provide clear separation:

  • CreateMigrationError for low-level file creation failures in the manager
  • NewMigrationError for use-case level errors exposed to CLI
mpt_tool/managers.py (1)

11-18: LGTM!

Import additions are clean and properly organized, bringing in the new error type and template for migration scaffolding.

mpt_tool/use_cases/new_migration.py (1)

12-21: LGTM!

The execute method properly:

  • Delegates to FileMigrationManager.new_migration()
  • Maps internal CreateMigrationError to user-facing NewMigrationError
  • Returns the generated filename for CLI feedback
mpt_tool/cli.py (3)

1-9: LGTM!

The imports are correctly structured and all added imports (cast, NewMigrationError, NewMigrationUseCase) are used in the code.


20-20: Static analysis false positive - noqa directive is valid for flake8.

The RUF100 hint flags WPS238 and WPS231 as unknown rules, but these are wemake-python-styleguide rules for flake8, not Ruff. Since the project uses both Ruff and flake8 (per pyproject.toml), the noqa directive is correctly placed for flake8 linting.


41-59: Options validation and migration execution logic look correct.

The mutual exclusivity check and at-least-one-option enforcement are properly implemented.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@svazquezco svazquezco force-pushed the MPT-16844-implement-new-migration-use-case branch from 37b53ac to 8fee1a9 Compare January 15, 2026 19:14
@svazquezco svazquezco force-pushed the MPT-16845-implement-migrate-data-and-schema-options branch from 84bf46c to bab4930 Compare January 16, 2026 11:51
@svazquezco svazquezco force-pushed the MPT-16844-implement-new-migration-use-case branch from 8fee1a9 to 20e3918 Compare January 16, 2026 12:00
@svazquezco svazquezco force-pushed the MPT-16845-implement-migrate-data-and-schema-options branch 2 times, most recently from 7cc9a10 to bc879bc Compare January 16, 2026 12:27
@svazquezco svazquezco force-pushed the MPT-16844-implement-new-migration-use-case branch from 20e3918 to 4f001ff Compare January 16, 2026 12:40
@svazquezco svazquezco marked this pull request as ready for review January 16, 2026 12:40
@svazquezco svazquezco marked this pull request as draft January 16, 2026 12:40
Base automatically changed from MPT-16845-implement-migrate-data-and-schema-options to main January 16, 2026 13:40
@svazquezco svazquezco force-pushed the MPT-16844-implement-new-migration-use-case branch from 4f001ff to fbf9eb9 Compare January 16, 2026 13:41
@svazquezco svazquezco marked this pull request as ready for review January 16, 2026 13:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@mpt_tool/cli.py`:
- Around line 65-69: The exception handlers for RunMigrationError and
NewMigrationError currently re-raise typer.Abort and thereby allow exception
chaining; update both handlers (the except blocks that catch RunMigrationError
around the run migration flow and the except block that catches
NewMigrationError around NewMigrationUseCase().execute) to use "raise
typer.Abort from None" to suppress exception chaining in CLI output.
♻️ Duplicate comments (2)
mpt_tool/models.py (1)

71-75: Validation message looks corrected.

Thanks for tightening the wording here.

mpt_tool/managers.py (1)

72-96: Line-length concern resolved; creation flow reads well.

The method is clear and the signature fits within the limit now.

📜 Review details

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37b53ac and fbf9eb9.

📒 Files selected for processing (5)
  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/managers.py
  • mpt_tool/models.py
  • mpt_tool/use_cases/new_migration.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: All configuration must be provided via environment variables; do not hardcode configuration values
Use type annotations (PEP 484) in Python files, except in the tests/ folder
All public functions, methods, and classes must include Google-style docstrings
Do not add inline comments; rely on clear code and docstrings instead
Function and variable names must be explicit and intention-revealing
Follow PEP 8 unless explicitly overridden by ruff

Files:

  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/cli.py
  • mpt_tool/managers.py

⚙️ CodeRabbit configuration file

**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.

Files:

  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/cli.py
  • mpt_tool/managers.py
**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/cli.py
  • mpt_tool/managers.py
🧠 Learnings (2)
📚 Learning: 2026-01-15T13:54:01.365Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:54:01.365Z
Learning: When reviewing Ruff lint violations, first check the project's pyproject.toml under [tool.ruff.lint].ignore to see which rules are explicitly ignored. Do not report violations for rules listed in that ignore list. Apply this guidance across all repositories in the softwareone-platform organization that use the same Ruff configuration.

Applied to files:

  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
  • mpt_tool/models.py
  • mpt_tool/cli.py
  • mpt_tool/managers.py
📚 Learning: 2026-01-15T13:30:45.197Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:30:45.197Z
Learning: In except blocks that raise typer.Abort, raise typer.Abort from None to suppress exception chaining (B904). Since Typer only prints 'Aborted!' and exits, ensure user-facing communication is done before raising Abort (e.g., via typer.secho) and apply this pattern in mpt_tool/cli.py to keep user messages clear.

Applied to files:

  • mpt_tool/cli.py
🧬 Code graph analysis (4)
mpt_tool/use_cases/new_migration.py (4)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/errors.py (2)
  • CreateMigrationError (12-13)
  • NewMigrationError (20-21)
mpt_tool/managers.py (2)
  • FileMigrationManager (21-96)
  • new_migration (73-96)
mpt_tool/models.py (1)
  • file_name (67-69)
mpt_tool/models.py (1)
mpt_tool/managers.py (1)
  • new (136-146)
mpt_tool/cli.py (5)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/errors.py (2)
  • NewMigrationError (20-21)
  • RunMigrationError (28-29)
mpt_tool/use_cases/run_migrations.py (2)
  • RunMigrationsUseCase (16-84)
  • execute (27-74)
mpt_tool/managers.py (1)
  • new_migration (73-96)
mpt_tool/use_cases/new_migration.py (2)
  • NewMigrationUseCase (6-21)
  • execute (12-21)
mpt_tool/managers.py (3)
mpt_tool/errors.py (4)
  • CreateMigrationError (12-13)
  • LoadMigrationError (16-17)
  • MigrationFolderError (24-25)
  • StateNotFoundError (32-33)
mpt_tool/models.py (4)
  • Migration (10-55)
  • MigrationFile (59-102)
  • new (93-102)
  • file_name (67-69)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
🪛 Ruff (0.14.11)
mpt_tool/models.py

73-75: Avoid specifying long messages outside the exception class

(TRY003)

mpt_tool/cli.py

20-20: Unused noqa directive (non-enabled: C901; unknown: WPS238, WPS231)

Remove unused noqa directive

(RUF100)

mpt_tool/managers.py

79-79: Avoid specifying long messages outside the exception class

(TRY003)


84-86: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (8)
mpt_tool/models.py (3)

66-69: file_name property is clear and consistent.

Nice centralization of the .py naming convention.


93-102: Factory method fits the new flow.

The timestamp-based name and returned instance line up well with the new scaffolding path.


71-75: No existing migration files found—validation concern is not applicable.

The repository contains no migration files to validate. The migrations/ directory does not exist, and no migration files are present anywhere in the codebase. All test fixtures use valid Python identifiers (fake_data_file_name, fake_error_file_name, fake_schema_file_name), which comply with the isidentifier() check. The validation is appropriate and necessary to prevent invalid identifiers (those containing hyphens or starting with digits) in future migration files.

mpt_tool/errors.py (1)

12-21: New error types improve boundary clarity.

These specialized exceptions make the migration scaffolding flow easier to reason about.

mpt_tool/managers.py (1)

11-18: Imports align with the new scaffolding flow.

Looks good and keeps dependencies explicit.

mpt_tool/use_cases/new_migration.py (1)

6-21: Use-case wrapper is straightforward.

The delegation and error translation are clean and consistent with the manager API.

mpt_tool/cli.py (2)

2-9: Imports updated cleanly for the new use case.

No issues here.


20-39: Option wiring looks good.

The new flags and exclusivity checks are clear and user-friendly.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@mpt_tool/cli.py`:
- Around line 63-69: The exception chain should be suppressed when aborting
after reporting the error: replace the plain raises of typer.Abort with "raise
typer.Abort from None" in both places where NewMigrationError is caught (the
except blocks that call typer.secho for the error message around the
NewMigrationUseCase().execute(...) call and the earlier similar block handling
filename_suffix), so that after logging the error you raise typer.Abort from
None to prevent exception chaining.

In `@mpt_tool/managers.py`:
- Around line 88-95: Wrap the call to migration_file.full_path.write_text(...)
in a try/except that catches OSError (and optionally IOError) and raises
CreateMigrationError with a clear message and the original exception chained
(raise CreateMigrationError(...) from e); update the block that builds the
scaffolding using MIGRATION_SCAFFOLDING_TEMPLATE and MigrationTypeEnum so that
any filesystem failures when writing the migration file for migration_file are
converted into CreateMigrationError instead of leaking an OSError.

In `@mpt_tool/models.py`:
- Around line 71-75: The new __post_init__ on MigrationFile can raise ValueError
when build_from_path parses legacy filenames; update
FileMigrationManager.validate() to catch ValueError coming from constructing
MigrationFile instances (or from MigrationFile.build_from_path) and re-raise a
MigrationFolderError that includes the offending filename (preserve the original
exception as __cause__), so RunMigrationsUseCase receives a MigrationFolderError
instead of an uncaught ValueError; reference the methods __post_init__,
build_from_path, FileMigrationManager.validate(), MigrationFolderError and
RunMigrationsUseCase when making the change.
📜 Review details

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37b53ac and fbf9eb9.

📒 Files selected for processing (5)
  • mpt_tool/cli.py
  • mpt_tool/errors.py
  • mpt_tool/managers.py
  • mpt_tool/models.py
  • mpt_tool/use_cases/new_migration.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • mpt_tool/errors.py
  • mpt_tool/use_cases/new_migration.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: All configuration must be provided via environment variables; do not hardcode configuration values
Use type annotations (PEP 484) in Python files, except in the tests/ folder
All public functions, methods, and classes must include Google-style docstrings
Do not add inline comments; rely on clear code and docstrings instead
Function and variable names must be explicit and intention-revealing
Follow PEP 8 unless explicitly overridden by ruff

Files:

  • mpt_tool/managers.py
  • mpt_tool/cli.py
  • mpt_tool/models.py

⚙️ CodeRabbit configuration file

**/*.py: Follow the linting rules defined in pyproject.toml under [tool.ruff] and [tool.flake8] sections.
For formatting, use Ruff instead of Black. Do not suggest Black formatting changes.

Files:

  • mpt_tool/managers.py
  • mpt_tool/cli.py
  • mpt_tool/models.py
**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

  • mpt_tool/managers.py
  • mpt_tool/cli.py
  • mpt_tool/models.py
🧠 Learnings (2)
📚 Learning: 2026-01-15T13:54:01.365Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:54:01.365Z
Learning: When reviewing Ruff lint violations, first check the project's pyproject.toml under [tool.ruff.lint].ignore to see which rules are explicitly ignored. Do not report violations for rules listed in that ignore list. Apply this guidance across all repositories in the softwareone-platform organization that use the same Ruff configuration.

Applied to files:

  • mpt_tool/managers.py
  • mpt_tool/cli.py
  • mpt_tool/models.py
📚 Learning: 2026-01-15T13:30:45.197Z
Learnt from: svazquezco
Repo: softwareone-platform/mpt-tool PR: 15
File: mpt_tool/cli.py:57-59
Timestamp: 2026-01-15T13:30:45.197Z
Learning: In except blocks that raise typer.Abort, raise typer.Abort from None to suppress exception chaining (B904). Since Typer only prints 'Aborted!' and exits, ensure user-facing communication is done before raising Abort (e.g., via typer.secho) and apply this pattern in mpt_tool/cli.py to keep user messages clear.

Applied to files:

  • mpt_tool/cli.py
🧬 Code graph analysis (2)
mpt_tool/cli.py (5)
mpt_tool/enums.py (1)
  • MigrationTypeEnum (4-13)
mpt_tool/errors.py (2)
  • NewMigrationError (20-21)
  • RunMigrationError (28-29)
mpt_tool/use_cases/run_migrations.py (2)
  • RunMigrationsUseCase (16-84)
  • execute (27-74)
mpt_tool/managers.py (1)
  • new_migration (73-96)
mpt_tool/use_cases/new_migration.py (2)
  • NewMigrationUseCase (6-21)
  • execute (12-21)
mpt_tool/models.py (1)
mpt_tool/managers.py (1)
  • new (136-146)
🪛 Ruff (0.14.11)
mpt_tool/managers.py

79-79: Avoid specifying long messages outside the exception class

(TRY003)


84-86: Avoid specifying long messages outside the exception class

(TRY003)

mpt_tool/cli.py

20-20: Unused noqa directive (non-enabled: C901; unknown: WPS238, WPS231)

Remove unused noqa directive

(RUF100)

mpt_tool/models.py

73-75: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (4)
mpt_tool/models.py (2)

66-70: file_name property is a clean addition.
Centralizes the .py suffix and avoids duplication.


93-102: Factory method looks good.
UTC timestamp + order_id alignment is clear and consistent with naming.

mpt_tool/managers.py (1)

72-74: Signature looks good; prior line-length concern resolved.
Line 73 is within the configured limit now.

mpt_tool/cli.py (1)

20-20: Keep the noqa comment for flake8/wemake compliance.

The noqa directive contains WPS238 and WPS231, which are flake8/wemake-python-styleguide rules. While Ruff will flag this as an unused noqa (RUF100), the directive must be retained because flake8 uses it for wemake rule suppression. Ruff does not recognize wemake rules, so it incorrectly marks them as unused—but they are necessary for linting compliance across both tools.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@svazquezco svazquezco force-pushed the MPT-16844-implement-new-migration-use-case branch from fbf9eb9 to f94281a Compare January 16, 2026 14:18
@svazquezco svazquezco force-pushed the MPT-16844-implement-new-migration-use-case branch from f94281a to da214a3 Compare January 16, 2026 14:20
@sonarqubecloud
Copy link

@d3rky d3rky merged commit 314fa76 into main Jan 16, 2026
4 checks passed
@d3rky d3rky deleted the MPT-16844-implement-new-migration-use-case branch January 16, 2026 14:43
@coderabbitai coderabbitai bot mentioned this pull request Feb 6, 2026
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.

2 participants