FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: refactor connect() function, cover it with unit tests by IlyaFaer · Pull Request #462 · googleapis/python-spanner-django · GitHub

This repository was archived by the owner on Jun 8, 2026. It is now read-only.
/ python-spanner-django Public archive

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

Merged
c24t merged 9 commits into
googleapis:masterfrom
MaxxleLLC:connect_refactor
Aug 27, 2020
ConversationCommits (9)ChecksFiles changed
Merged

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

Conversation

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 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 Bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 21, 2020
IlyaFaer commented Aug 21, 2020
View reviewed changes
Comment thread tests/spanner_dbapi/test_connect.py Outdated
IlyaFaer requested a review from c24t August 21, 2020 09:34
IlyaFaer marked this pull request as ready for review August 21, 2020 09:34

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

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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 commented Aug 25, 2020
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 commented Aug 27, 2020
View reviewed changes
Comment thread 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"],

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

c24t merged commit bfde221 into googleapis:master Aug 27, 2020
IlyaFaer deleted the connect_refactor branch August 28, 2020 07:13
c24t pushed a commit that referenced this pull request Sep 15, 2020
feat: refactor connect() function, cover it with unit tests (#462)
4fedcf1
This was referenced Apr 17, 2024
Closed
Closed
Closed
Closed
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
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

Back | FazBrowse Home | New Git URL