-
-
Notifications
You must be signed in to change notification settings - Fork 261
feat(migrate): implement migration sort customization #1186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 3 commits
60f07c3
5a70b4e
3de66a6
43ec518
5b9b52c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,7 +38,7 @@ func NewMigrations(opts ...MigrationsOption) *Migrations { | |
| func (m *Migrations) Sorted() MigrationSlice { | ||
| migrations := make(MigrationSlice, len(m.ms)) | ||
| copy(migrations, m.ms) | ||
| sortAsc(migrations) | ||
| SafeAscSort(migrations) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is the only place where sorting needs to be applied, could we allow users to pass in a sorting function when calling it, making the parameter optional? If we do that, we can store the sorting logic in the migrator, thereby avoiding the need to implement any locking-related logic.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a breaking change. You said you breaking changes weren't allowed...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @j2gg0s do you mean passing as an option to NewMigrator? Because I don't think users ever call ms.Sorted directly so something like ms.SortedFunc(fn sort.Less) probably won't be that convenient. Personally, I think having a package-level sorting is fine -- seems like something you'd only want to set once. Re: maintenance, the overhead of that locking is immaterial + we needn't configure it for AutoMigrator separately. wdyt?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ping @j2gg0s |
||
| return migrations | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| package migrate | ||
|
|
||
| import ( | ||
| "sort" | ||
| "sync" | ||
| ) | ||
|
|
||
| // MigrationSortFunc defines the signature for functions that sort migrations. | ||
| type MigrationSortFunc func(ms MigrationSlice) | ||
|
|
||
| // sortMutex protects access to the global sort functions. | ||
| var sortMutex sync.RWMutex | ||
|
|
||
| // Default sort implementations | ||
| var defaultAscSort MigrationSortFunc = func(ms MigrationSlice) { | ||
| sort.Slice(ms, func(i, j int) bool { | ||
| return ms[i].Name < ms[j].Name | ||
| }) | ||
| } | ||
|
|
||
| var defaultDescSort MigrationSortFunc = func(ms MigrationSlice) { | ||
| sort.Slice(ms, func(i, j int) bool { | ||
| return ms[i].Name > ms[j].Name | ||
| }) | ||
| } | ||
|
|
||
| // AscSort is the global ascending sort function. | ||
| // Default is to sort by migration name in ascending order. | ||
| // Can be overridden to use custom sorting logic. | ||
| var AscSort MigrationSortFunc = defaultAscSort | ||
|
|
||
| // DescSort is the global descending sort function. | ||
| // Default is to sort by migration name in descending order. | ||
| // Can be overridden to use custom sorting logic. | ||
| var DescSort MigrationSortFunc = defaultDescSort | ||
|
|
||
| // SafeAscSort applies the current ascending sort function in a thread-safe manner. | ||
| func SafeAscSort(ms MigrationSlice) { | ||
| sortMutex.RLock() | ||
| defer sortMutex.RUnlock() | ||
| AscSort(ms) | ||
| } | ||
|
|
||
| // SafeDescSort applies the current descending sort function in a thread-safe manner. | ||
| func SafeDescSort(ms MigrationSlice) { | ||
| sortMutex.RLock() | ||
| defer sortMutex.RUnlock() | ||
| DescSort(ms) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that MigrationSlice.Applied is no longer used internally in Bun?