Skip to content

Conversation

@jayvdb
Copy link
Collaborator

@jayvdb jayvdb commented Oct 28, 2025

Semi-related to #91

A common case is a migration has been committed on a PR, but needs amending before approval & merging.

Comment on lines +74 to +75
/// Merge the most recent migration into the previous migration.
Squash,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this be SquashMigration ?

Copy link
Owner

Choose a reason for hiding this comment

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

I think I have a slight preference for SquashMigration

@jayvdb jayvdb marked this pull request as ready for review October 28, 2025 12:13
@jayvdb jayvdb requested a review from Electron100 October 28, 2025 12:13
Comment on lines +677 to +678
// Create a new migration that represents the squashed state, using the name of the second-to-last migration
ms.create_migration_to(&backends, &second_to_last_name, from_migration, target_db)?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This overwrites the .sql in the previous migration.

Maybe we need a prompt to ensure users understand this.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I think this is the right behavior, but probably good to introduce user confirmation for (ideally with a flag to override the prompt if necessary to use it in a fully automated fashion)

Comment on lines +74 to +75
/// Merge the most recent migration into the previous migration.
Squash,
Copy link
Owner

Choose a reason for hiding this comment

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

I think I have a slight preference for SquashMigration

// Create a new migration that represents the squashed state, using the name of the second-to-last migration
ms.create_migration_to(&backends, &second_to_last_name, from_migration, target_db)?;

// Delete the old migration directories from the filesystem
Copy link
Owner

Choose a reason for hiding this comment

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

[nit] It's only one directory we're deleting, not plural directories

Comment on lines +677 to +678
// Create a new migration that represents the squashed state, using the name of the second-to-last migration
ms.create_migration_to(&backends, &second_to_last_name, from_migration, target_db)?;
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I think this is the right behavior, but probably good to introduce user confirmation for (ideally with a flag to override the prompt if necessary to use it in a fully automated fashion)

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