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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from Aug 27, 2020
Merged

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

merged 9 commits into from Aug 27, 2020

Conversation

IlyaFaer
Copy link
Contributor

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 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
Copy link
Contributor Author

IlyaFaer commented Aug 21, 2020 •
edited

@c24t, isort check failed in kokoro. It looks like isort conflicts with black (the formatter we're using in Google Python clients, see config). They sort imports in different ways.

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)?

@IlyaFaer IlyaFaer mentioned this pull request Aug 24, 2020
Merged
c24t
c24t reviewed Aug 24, 2020
View reviewed changes
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.

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

spanner_dbapi/__init__.py Show resolved Hide resolved
spanner_dbapi/__init__.py Show resolved Hide resolved
spanner_dbapi/__init__.py Outdated Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Outdated Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Outdated Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Outdated Show resolved Hide resolved
@c24t
Copy link
Contributor

c24t commented Aug 24, 2020

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
Copy link
Contributor Author

@IlyaFaer IlyaFaer left a comment

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

spanner_dbapi/__init__.py Show resolved Hide resolved
spanner_dbapi/__init__.py Outdated Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Outdated Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Outdated Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Show resolved Hide resolved
tests/spanner_dbapi/test_connect.py Outdated Show resolved Hide resolved
IlyaFaer
IlyaFaer commented Aug 27, 2020
View reviewed changes
django_spanner/base.py
**self.settings_dict['OPTIONS'],
"project": self.settings_dict["PROJECT"],
"instance_id": self.settings_dict["INSTANCE"],
"database_id": self.settings_dict["NAME"],
Copy link
Contributor 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 join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c24t c24t c24t approved these changes

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