Change ID Database #4247
Change ID Database #4247
Conversation
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Let's have integration tests, that is tests for public APIs go into <crate>/tests/ similar to how it's done in core. It's actually a good check to assure public APIs are actually usable from other crates as well.
| const DATABASE_NAME: &str = "project.sqlite"; | ||
|
|
||
| /// ProjectStore provides a light wrapper around a sqlite database | ||
| struct ProjectStore { |
There was a problem hiding this comment.
Since Connection is !Sync, i.e. not Sync, the whole ProjectStore will have to be behind a Mutex to compensate for that.
If Clone is implemented such that it connects to the database without rerunning initialization and migration then it concurrency will be possible nonetheless, and it can be as granular as sqlite can make it.
| "; | ||
|
|
||
| /// Migrator is used to perform migrations on a particular database. | ||
| pub(crate) struct Migrator<'l> { |
There was a problem hiding this comment.
When there is no name used for the lifetime then it's traditionally 'a, which is better for readability and sticking to conventions. Then I for one don't have to wonder if l means link, but be upset because I will never know for sure 😁.
| let migrator = Migrator::new(&mut connection); | ||
|
|
||
| // Try to find unapplied migrations | ||
| migrator.find_applied_migrations().unwrap(); |
There was a problem hiding this comment.
There should probably be an assertion here to assure they are empty.
| fn find_applied_migrations_lists_all_entries_in_table() { | ||
| let mut connection = Connection::open_in_memory().unwrap(); | ||
|
|
||
| // Call find_applied_migrations in order to create the migrations table |
There was a problem hiding this comment.
I'd move this comment into an assertion as last argument of assert! for example:
assert!(migrator.find_applied_migrations().unwrap().is_empty(), "nothing happened yet, but creates table as side-effect");| #[test] | ||
| /// Testing the `migrate` function. | ||
| /// When given a list of migrations to run, it will perform the migrations in order | ||
| fn migrate_applies_migrations_in_order() { |
There was a problem hiding this comment.
I recommend to not test find_applied_migrations() as it needs to know a lot of details.
Instead, let migrate() return enough information to know which migrations were applied. That way there can be a 'journey' that does what happens typically, empty database, then the first migration, then the second, with idempotency checks sprinkled in.
Tests that are considered good because they don't panic are an indicator that some more useful information could be returned organically to make what happens more observable.
| /// If the migration has already been run, it will be skipped. | ||
| /// Run migrations get recorded in the `migrations` table. | ||
| /// The `migrations` table will be created if it doesn't exist. | ||
| pub(crate) fn migrate(&mut self, migrations: Vec<Migration>) -> Result<()> { |
There was a problem hiding this comment.
migrations doesn't need to be consumed, and when I read it I wonder why that needs to be consumed. Better not 'lie' and do &[Migration] instead.
| ))?; | ||
|
|
||
| let connection = Connection::open(database_path)?; | ||
|
|
There was a problem hiding this comment.
This whitespace puzzles me, even though I know that I had a phase where I did that too.
|
@Caleb-T-Owens do you still intend to work on this pr? |
|
@mtsgrd At some point I think it would be good to introduce a SQL database. When I started we had a product-related reason for doing it (we had data we wanted to associate to commits/changes) but we ended up representing the data differently, so its no longer a priority. |
No description provided.