Skip to content
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

Merged
merged 8 commits into from Dec 17, 2020

Conversation

mf2199
Copy link
Contributor

@mf2199 mf2199 commented Oct 25, 2020

Change list:

  1. Added implementation of DatabaseWrapper.init_connection_state() method;
  2. Potential bug fix in the DatabaseWrappper.is_usable() condition check;
  3. Miscellaneous refactoring and reformatting.

…in_usable()` methods of the `DatabaseWrapper` class
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 25, 2020
@mf2199 mf2199 requested a review from IlyaFaer October 25, 2020 00:45
@mf2199 mf2199 added api: spanner Issues related to the googleapis/python-spanner-django API. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 25, 2020
@mf2199 mf2199 marked this pull request as ready for review October 26, 2020 18:55
@mf2199 mf2199 requested a review from a team as a code owner October 26, 2020 18:55
@mf2199 mf2199 requested a review from c24t October 26, 2020 18:56
Copy link
Contributor

@c24t c24t left a 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?

Comment on lines 87 to 88
# These are all no-ops in favor of using REGEXP_CONTAINS
# in the customized lookups.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# 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?

django_spanner/base.py Outdated Show resolved Hide resolved
django_spanner/base.py Show resolved Hide resolved
version.py Outdated Show resolved Hide resolved


@mock_import()
@unittest.skipIf(sys.version_info < (3, 6), reason="Skipping Python 3.5")
Copy link
Contributor

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.

Copy link
Contributor

@c24t c24t left a 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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner-django API. cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants