New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: missing method implementation and potential bug in the DatabaseWrapper
class
#545
Conversation
…in_usable()` methods of the `DatabaseWrapper` class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the potential bug this fixes? Do you expect init_connection_state
to get called after initializing the wrapper?
Can you add a test that fails on master but passes with this change?
django_spanner/base.py
Outdated
# These are all no-ops in favor of using REGEXP_CONTAINS | ||
# in the customized lookups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# These are all no-ops in favor of using REGEXP_CONTAINS | |
# in the customized lookups. | |
# These are all no-ops in favor of using REGEXP_CONTAINS in the customized | |
# lookups. |
Why rewrap this?
|
||
|
||
@mock_import() | ||
@unittest.skipIf(sys.version_info < (3, 6), reason="Skipping Python 3.5") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can safely drop 3.5 in tests since we already decided not to support it elsewhere.
7f1dd87
to
29652ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me without requiring django 3.x.
@@ -92,7 +95,7 @@ class DatabaseWrapper(BaseDatabaseWrapper): | |||
"iendswith": "", | |||
} | |||
|
|||
Database = Database | |||
Database = spanner_dbapi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this odd "import x as Database; Database = Database" pattern seems to come from sqlite (https://github.com/django/django/blob/e893c0ad8b0b5b0a1e5be3345c287044868effc4/django/db/backends/sqlite3/base.py#L156), and is copied in all the other built in backends.
Change list:
DatabaseWrapper.init_connection_state()
method;DatabaseWrappper.is_usable()
condition check;