Import sqlite3 only if used; minor bugfixes #620
Conversation
Zac-HD
force-pushed
the
conditional-sqlite
branch
from
10fb3e2 to
a6f3243
Compare
Zac-HD
force-pushed
the
conditional-sqlite
branch
from
a6f3243 to
2f20f98
Compare
Zac-HD
force-pushed
the
conditional-sqlite
branch
2 times, most recently
from
6329368 to
0f9081e
Compare
Zac-HD
changed the title
There was a problem hiding this comment.
I think we should resist the temptation to bundle multiple fixes into a single PR just so they go in a single release, so I'd like to split this out into two PRs with their own patch release.
The individual changes look fine, but I'd like some tests for them - I'm not at all clear on how we'd test the sqlite3 logic right now, so if we can't think of something then I'm happy to leave it, but the enum change should definitely be tested.
Sure. I just find it frustrating to write competing changelogs for a single proposed version - is there anything I can do to help review go faster? |
Zac-HD
force-pushed
the
conditional-sqlite
branch
from
0f9081e to
3cc95a1
Compare
|
Also unsure how to test this, but pulls have been split. Any other blockers? |
There was a problem hiding this comment.
I just find it frustrating to write competing changelogs for a single proposed version
I agree. I think some means of automating changelog aggregation and version management is increasingly necessary. I'm still pondering what the best thing to do is here, but am very open to suggestions.
is there anything I can do to help review go faster?
I think not really - the major blocker on review is just that @alexwlchan and I are both quite busy on other things right now, and we're the main people who could review.
docs/changes.rst
Outdated
|
|
||
| Instead of unconditionally importing ``sqlite3``, Hypothesis now imports it | ||
| when a SQLite database is first used. This was the major blocker for BSD | ||
| support with default settings. |
There was a problem hiding this comment.
I'd probably just say "this allows Hypothesis to work on implementations compiled without sqlite support" or similar. It's not really a major blocker because the fix is just "install sqlite support".
Zac-HD
force-pushed
the
conditional-sqlite
branch
from
3cc95a1 to
573bc62
Compare
docs/changes.rst
Outdated
|
|
||
| This is a compatibility release: Hypothesis no longer imports ``sqlite3`` | ||
| before a SQLite database is first used, so the default settings now work on | ||
| Python implementations compiled without sqlite support (eg BSD). |
There was a problem hiding this comment.
I refuse to make this a blocking comment and am going to approve the change, but my preferred convention here is "e.g. " rather than "eg" and it would make me happy if you followed. :-)
|
LGTM. Note that you'll need to rebase against master before it's mergeable. |
|
And bump the date in the changelog! |
Zac-HD
force-pushed
the
conditional-sqlite
branch
from
573bc62 to
e67a5bb
Compare
|
@DRMacIver, after #629 is merged we can rerun the changelog check and merge. |
Still a single import, but this way it's only attempted if you're actually using a sqlite example database.
Zac-HD
force-pushed
the
conditional-sqlite
branch
from
e67a5bb to
775084a
Compare
Still a single import, but this way it's only attempted if you're actually using a sqlite example database. IMO it is better to raise an import error if you attempt to use sqlite than to change the database selection logic depending on whether it is installed. Closes #173.