Open
Conversation
rubenv
reviewed
Jan 27, 2022
migrate.go
Outdated
Comment on lines
124
to
144
| type Migration struct { | ||
| Id string | ||
| Up []string | ||
| Down []string | ||
|
|
||
| type MigrationData struct { | ||
| Up []string | ||
| Down []string | ||
| DisableTransactionUp bool | ||
| DisableTransactionDown bool | ||
| } | ||
|
|
||
| type migrationFile struct { | ||
| dir http.FileSystem | ||
| root string | ||
| baseName string | ||
| } | ||
|
|
||
| type Migration struct { | ||
| Id string | ||
|
|
||
| // data may be nil if not parsed yet. | ||
| data *MigrationData | ||
| // file is information of migration file, which is used to parse later. | ||
| // file may be nil if migration file has already been parsed when Migration is initialized. | ||
| file *migrationFile | ||
| } |
Owner
There was a problem hiding this comment.
This is a problem really. It changes the external API of the library, which means it may break peoples code. And I'd rather not do that.
Unless we find a way to do this without breaking the API, I'm afraid this won't be an option.
Author
This reverts commit 859f73b.
…le to be loaded only when needed.
KeiichiHirobe
force-pushed
the
lazy-load
branch
from
ca2e507 to
14223b2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current implementation always parse all of sql files, so may require a lot of time and memory if there are too many files or too big files in directory. But, in most of cases, we only need to parse 1~3 files because sort
byIddoes not need content of files, it needs only the name of files.So, I think it is better to parse sql files only when we need content of files.
Some migration sources are not supported to open/parse lazily in this PR.
HttpFileSystemMigrationSource,FileMigrationSourceandEmbedFileSystemMigrationSourceare supported,MemoryMigrationSource,AssetMigrationSourceandPackrBoxare not supported.I don't think that's a problem because I guess most users don't use the latter migration sources.
I believe lazy loading should be the default behavior, but I made it optional with
SetLazyLoad(true)to keep backward compatibility.ref: #203