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

@IlyaFaerIlyaFaer 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

@IlyaFaerIlyaFaer 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-clagoogle-claBot 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 threadtests/spanner_dbapi/test_connect.py Outdated
@IlyaFaer
IlyaFaer requested a review from c24tAugust 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

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

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

@c24tc24t 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 threadspanner_dbapi/__init__.py
Comment threadspanner_dbapi/__init__.py
Comment threadspanner_dbapi/__init__.py Outdated
Comment threadtests/spanner_dbapi/test_connect.py Outdated
Comment threadtests/spanner_dbapi/test_connect.py
Comment threadtests/spanner_dbapi/test_connect.py Outdated
Comment threadtests/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

@IlyaFaerIlyaFaer 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 threadspanner_dbapi/__init__.py
Comment threadspanner_dbapi/__init__.py Outdated
Comment threadtests/spanner_dbapi/test_connect.py Outdated
Comment threadtests/spanner_dbapi/test_connect.py Outdated
Comment threadtests/spanner_dbapi/test_connect.py
Comment threadtests/spanner_dbapi/test_connect.py Outdated
IlyaFaer
IlyaFaer commented Aug 27, 2020
View reviewed changes
Comment threaddjango_spanner/base.py
**self.settings_dict['OPTIONS'],
"project": self.settings_dict["PROJECT"],
"instance_id": self.settings_dict["INSTANCE"],
"database_id": self.settings_dict["NAME"],

@IlyaFaerIlyaFaerAug 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:masterAug 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
Gurov Ilya@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 freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

Assignees

No one assigned

Labels

api: spannerIssues related to the googleapis/python-spanner-django API.cla: yesThis 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