Make fn Migrations::max_schema_version public.
#171
Conversation
|
@fluffysquirrels thanks for your PR, I've suggested a few improvements. The most important one is adding the same change to the async part of the crate (unfortunately, these things have to be manually synced). Please feel free to challenge those suggestions. |
|
Overall, lgtm, but let's hold merging a little bit: I've posted a comment to further discuss alternative approaches |
Cool. Your changes look good. I will probably update the PR with a few suggestions from issue #170 so we can see how it looks. |
Just pushed a new version with this. I also wrapped AsyncMigrations.migrations in an |
| /// | ||
| /// For most common scenarios, you should be able to just call | ||
| /// [`Migrations::to_latest`], which already checks the schema version. | ||
| /// |
There was a problem hiding this comment.
If we do decide to add this method, might be worth adding a code example with the if.
Maybe also worth having a full example of an actual backup process to make sure that there is nothing awkward with the lifetimes.
| warn!("no migration defined"); | ||
| Err(Error::MigrationDefinition( | ||
| MigrationDefinitionError::NoMigrationsDefined, | ||
| )) | ||
| } | ||
| SchemaVersion::Inside(v) => { |
There was a problem hiding this comment.
I would prefer to keep the SchemaVersion type here. It's not just any integer, it's a schema version and those are off by one, effectively, from the index in the migrations vector.
| warn!("no migrations defined"); | ||
| Err(Error::MigrationDefinition( | ||
| MigrationDefinitionError::NoMigrationsDefined, | ||
| )) | ||
| } | ||
| SchemaVersion::Inside(v) => { |
There was a problem hiding this comment.
Same here, let’s keep SchemaVersion.
|
@fluffysquirrels I’ve incorporated the Arc change separately to master and fixed some clippy warnings that came up with the latest rust version. Feel free to squash your commits together when you rebase. |
We have to keep the APIs exposed by the two structs as in sync as possible.
Here, we are telling the compiler that we have already performed a check even though it can’t seem to detect it (for instance, clippy emits a warning because of the expect).
cljoly
force-pushed
the
pub-max_schema_version
branch
from
ae25dbd to
61f95de
Compare
|
Rebased on master |
|
As this is not quite ready yet, I’ve pushed it back to the next stable version 1.4.0 (which could be published as soon as this is ready, potentially). |
Addresses #170
fn Migrations::max_schema_version(&self)is a currently private method that returns the number of available migrations wrapped in SchemaVersion.I would like access to this value in my own code. I'm using
Migrations::from_directory()andinclude_dir!to construct my Migrations, so getting the count myself is non-trivial AFAICT.My use case is to work out whether any migrations will be run by
fn Migrations::to_latest()before running it, and if so to take a backup first.The library code change is effectively 1 line (1 more to disable a clippy lint on the now-public method, 1 more to improve documentation IMO). Since this adds a new method to the public API I put 2 asserts into an existing test to avoid regressions.