Add make_reader(migrate=False) and matching CLI option - #415
Add make_reader(migrate=False) and matching CLI option #415divyanshi555 wants to merge 1 commit into
Conversation
|
Hi @lemon24 ! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #415 +/- ##
=======================================
Coverage 94.04% 94.05%
=======================================
Files 110 110
Lines 14043 14065 +22
Branches 1040 1041 +1
=======================================
+ Hits 13207 13229 +22
Misses 756 756
Partials 80 80 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lemon24
left a comment
lemon24
left a comment
There was a problem hiding this comment.
Thank you for the PR, looks good! (and thanks for testing everything locally)
I added a few small comments, after they're addressed we're good to merge.
| # Build a database stuck at version 1 | ||
| old_db_path = str(pathlib.Path(db_path).parent / 'old.sqlite') | ||
| db = sqlite3.connect(old_db_path) | ||
| try: | ||
| HeavyMigration(create=lambda db: None, version=1, migrations={}).migrate(db) | ||
| finally: | ||
| db.close() | ||
|
|
||
| # Define a new schema (version 2) with trivial migrations from version 1 to 2 | ||
| new_migration = HeavyMigration( | ||
| create=lambda db: None, | ||
| version=2, | ||
| migrations={1: lambda db: None}, | ||
| ) | ||
| monkeypatch.setattr('reader._storage._schema.MIGRATION', new_migration) |
There was a problem hiding this comment.
I think increasing the version can be simplified a bit by patching MIGRATION from the beginning:
migration = HeavyMigration(create=lambda db: None, version=1, migrations={})
monkeypatch.setattr('reader._storage._schema.MIGRATION', migration)
# ... test empty case
with make_reader(db_path): pass
migration.version = 2
migration.migrations[1] = lambda _: None
# ... test make_reader(db_path, migrate=False)
# ... test make_reader(db_path)| # migrate=False, refuse and raise StorageError. | ||
| with pytest.raises(StorageError) as excinfo: | ||
| make_reader(old_db_path, migrate=False) | ||
| assert 'migrate=False' in str(excinfo.value) |
There was a problem hiding this comment.
Besides the error (or lack thereof), it's a good idea to check if the database actually changed or not:
with make_reader(db_path, ...) as reader:
assert migration.get_version(reader._storage.get_db()) == ...| path: str, | ||
| read_only: bool = False, | ||
| timeout: float | None = None, | ||
| migrate: bool = True, |
There was a problem hiding this comment.
nit: Might be better to put migrate after read_only, to keep "how to create this" flags together, and for symmetry with make_reader().
2 participants
Fixes #403
Adds a
migrateflag tomake_reader()(defaultTrue) and a matching--migrate/--no-migrateCLI flag.Whenmigrate=Falseand a migration is needed,StorageErroris raised instead of migrating, done by passingsetup_db()a copy ofMIGRATIONwith an emptymigrationsdict and a custommissing_suffix, as suggested in the issue.Changes :
_storage/_base.py:StorageBase.setup_db()acceptsmigrate; buildsan empty-migrations copy of
MIGRATIONwith a custom error suffixwhen
migrate=False._storage/__init__.py:Storage.__init__threadsmigratethrough.core.py:make_reader()gains themigrateparameter + docstring._cli.py: adds--migrate/--no-migrate(defaultTrue).tests/test_reader_lifecycle.py: newtest_migrate, verifyingStorageErroris raised whenmigrate=Falseand a migration isneeded, and that
migrate=Truestill migrates normally.Verification
Ran the full test suite, coverage, mypy strict, and the docs build all passing locally.