chore: unit test coverage and additional refactoring for spanner_dbapi
#532
Conversation
# Conflicts: # google/cloud/spanner_dbapi/__init__.py # google/cloud/spanner_dbapi/connection.py # tests/spanner_dbapi/test_connection.py
cursor.pyspanner_dbapi
There was a problem hiding this comment.
All the changes here look sound individually, but putting them in one big PR like this does make it hard to review (and may cause painful merge conflicts for other branches in flight since this will be squashed into a single commit on master). It's possible that I missed something, but LGTM otherwise.
| ) | ||
| from .version import google_client_info | ||
| from google.cloud.spanner_dbapi.connection import Connection | ||
| from google.cloud.spanner_dbapi.connection import connect |
There was a problem hiding this comment.
FWIW our current isort config will mangle this formatting. Did you do this by hand?
There was a problem hiding this comment.
Yes, I did. It would work either way, and was more of the matter of a consistent visual appearance.
| # TODO: File a bug with Cloud Spanner and the Python client maintainers | ||
| # about a lost commit when res isn't read from. |
There was a problem hiding this comment.
I know your change just moved this comment, but I wonder what the story is here.
There was a problem hiding this comment.
It would be a good question for the original code developer who unfortunately is no longer a member of our team. I'm not even sure what's the purpose of res, as the function does not return anything. We should probably revise this method later.
|
|
||
| # pyformat_str_count = after_values_sql.count("%s") | ||
| # if pyformat_str_count > 0: | ||
| # raise ProgrammingError( | ||
| # 'no params yet there are %d "%%s" tokens' % pyformat_str_count | ||
| # ) |
There was a problem hiding this comment.
| # pyformat_str_count = after_values_sql.count("%s") | |
| # if pyformat_str_count > 0: | |
| # raise ProgrammingError( | |
| # 'no params yet there are %d "%%s" tokens' % pyformat_str_count | |
| # ) |
LGTM, no reason to keep the old code around as far as I can tell.
There was a problem hiding this comment.
This has already been addressed in a separate PR - precisely for the reason mentioned earlier, that is not to make this already large PR even bigger.
| # def test_handle_insert(self): | ||
| # pass | ||
| # | ||
| # def test_do_execute_insert_heterogenous(self): | ||
| # pass | ||
| # | ||
| # def test_do_execute_insert_homogenous(self): | ||
| # pass |
There was a problem hiding this comment.
Looks like these are new tests, why add them as commented stubs here?
There was a problem hiding this comment.
This has also been addressed in a separate PR.
…on-spanner-django into q-logic-refactor-cursor-alt # Conflicts: # google/cloud/spanner_dbapi/__init__.py # google/cloud/spanner_dbapi/connection.py # google/cloud/spanner_dbapi/cursor.py # tests/spanner_dbapi/test_connect.py # tests/spanner_dbapi/test_connection.py
Q logic refactor cursor alt
|
@olavloite Thanks for the review. Made all the changes as suggested. PTAL. |
Listed below is the summary of the proposed changes.
1. Refactoring the contents of
spanner_dbapipackage:ColumnInfoclass and few static methods not listed in PEP-249 moved fromcursor.pyinto a new_helpers.pyfile;connectmoved from__init__.pyintoconnection.py;connection.pyandcursor.pypartially revised and updated;andCursor` classes was updated to follow that of PEP-249 definitions;parse_utils.parse_insert()method (the old code left commented-out for now);parser.py;version.pydefinitions;2. Improving the unit test coverage level up to 100%:
spanner_dbapi;django_spannerwas temporarily excluded from the coverage since it's covered by Django system tests anyway;tests/spanner_dbapifolder refactored intotests/unit/spanner_dbapias to follow Spanner's common folder structure;tox.inifile removed as deprecated since the switch to the Nox framework.