feat: refactor connect() function, cover it with unit tests by IlyaFaer · Pull Request #462 · googleapis/python-spanner-django · GitHub
Skip to content
This repository was archived by the owner on Jun 8, 2026. It is now read-only.

feat: refactor connect() function, cover it with unit tests#462

Merged
c24t merged 9 commits into
googleapis:masterfrom
MaxxleLLC:connect_refactor
Aug 27, 2020
Merged

feat: refactor connect() function, cover it with unit tests#462
c24t merged 9 commits into
googleapis:masterfrom
MaxxleLLC:connect_refactor

Conversation

@IlyaFaer

@IlyaFaer IlyaFaer commented Aug 21, 2020

Copy link
Copy Markdown

Run formatters.
Fit doclines, variable names and function args to the original Spanner client fashion.
Cover the function with unit tests.

Closes #460

@IlyaFaer IlyaFaer added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: spanner Issues related to the googleapis/python-spanner-django API. labels Aug 21, 2020
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 21, 2020
IlyaFaer
IlyaFaer commented Aug 21, 2020
View reviewed changes
Comment thread tests/spanner_dbapi/test_connect.py Outdated
@IlyaFaer IlyaFaer requested a review from c24t August 21, 2020 09:34
@IlyaFaer IlyaFaer marked this pull request as ready for review August 21, 2020 09:34
@IlyaFaer

IlyaFaer commented Aug 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Author

@IlyaFaer IlyaFaer mentioned this pull request Aug 24, 2020
Merged
c24t
c24t reviewed Aug 24, 2020
View reviewed changes

@c24t c24t left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor blocking comments, but the changes look great otherwise.

Comment thread spanner_dbapi/__init__.py
Comment thread spanner_dbapi/__init__.py
Comment thread spanner_dbapi/__init__.py Outdated
Comment thread tests/spanner_dbapi/test_connect.py Outdated
Comment thread tests/spanner_dbapi/test_connect.py
Comment thread tests/spanner_dbapi/test_connect.py Outdated
Comment thread tests/spanner_dbapi/test_connect.py Outdated
@c24t

c24t commented Aug 24, 2020

Copy link
Copy Markdown
Contributor

I don't see isort mentions in the original Spanner client. Maybe we should redact tox to run black instead of isort (in this PR probably)?

Another option is to change isort's config to appease black.

[settings]
include_trailing_comma=True
force_grid_wrap=0
use_parentheses=True
line_length=79
multi_line_output=3
skip=target
skip_glob=...
known_first_party=...
known_third_party=...

We had to do this in another project here: open-telemetry/opentelemetry-python#104 (comment).

IlyaFaer
IlyaFaer commented Aug 25, 2020
View reviewed changes

@IlyaFaer IlyaFaer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@c24t, looks like comments are mostly about things that I'm just mimicking from other Cloud repos. It's not my preferences, so I'm ready to do however you (as a googler) decide

Comment thread spanner_dbapi/__init__.py
Comment thread spanner_dbapi/__init__.py Outdated
Comment thread tests/spanner_dbapi/test_connect.py Outdated
Comment thread tests/spanner_dbapi/test_connect.py Outdated
Comment thread tests/spanner_dbapi/test_connect.py
Comment thread tests/spanner_dbapi/test_connect.py Outdated
IlyaFaer
IlyaFaer commented Aug 27, 2020
View reviewed changes
Comment thread django_spanner/base.py

@IlyaFaer IlyaFaer Aug 27, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed arg names here, other changes in the file are just about formatting

@c24t c24t merged commit bfde221 into googleapis:master Aug 27, 2020
@IlyaFaer IlyaFaer deleted the connect_refactor branch August 28, 2020 07:13
c24t pushed a commit that referenced this pull request Sep 15, 2020
@c24t
feat: refactor connect() function, cover it with unit tests (#462)
4fedcf1
This was referenced Apr 17, 2024
Closed
Closed
Closed
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@c24t c24t c24t approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: spanner Issues related to the googleapis/python-spanner-django API. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

connect() surface issues

2 participants

@IlyaFaer @c24t