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.

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

connect() surface issues #460

Closed
IlyaFaer opened this issue Aug 20, 2020 · 1 comment · Fixed by #462
Closed

connect() surface issues #460

IlyaFaer opened this issue Aug 20, 2020 · 1 comment · Fixed by #462
Assignees
Labels
api: spanner Issues related to the googleapis/python-spanner-django API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@IlyaFaer
Copy link
Contributor

Looking at connect() function, there are several things which are raising questions.

project arg:
project is set as an optional arg:

def connect(project=None, instance=None, database=None, credentials_uri=None, user_agent=None):
But few lines lower it is checked for existence (thus, it's required, not optional):
if not project:
raise Error("'project' is required.")
It seems to be pointless, as this arg is only passed to Spanner client, and the original Spanner client can handle an omitted project: it'll use google.auth.default() function to designate the project id from the environment. With this, I'd propose to drop this check and use project arg in the way of the original Spanner client.

instance and database args:
They are optional by definition, but in fact both are required. Probably, they should become required in definition (the function signature will be connect(instance, database, project=None, credentials_uri=None, user_agent=None):), so these checks could be dropped:

if not instance:
raise Error("'instance' is required.")
if not database:
raise Error("'database' is required.")

credentials_uri arg:
This arg looks divergent from how the original Spanner client is dealing with credentials:
https://github.com/googleapis/python-spanner/blob/b6b0348f5870f55c88b3cae44e229325fd755b26/google/cloud/spanner_v1/client.py#L123-L129

I'd propose to change this arg to use google.auth.credentials.Credentials class as it's done in the original Spanner client.

@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 20, 2020
@IlyaFaer IlyaFaer self-assigned this Aug 20, 2020
@c24t
Copy link
Contributor

c24t commented Aug 20, 2020

I agree with everything here, the null default args don't make and aren't required by DBAPI. And better to use Credentials than credentials_uri.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner-django API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants