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

docs: Docstrings for django_spanner #564

Merged
merged 36 commits into from
Dec 16, 2020
Merged

Conversation

mf2199
Copy link
Contributor

@mf2199 mf2199 commented Dec 3, 2020

No description provided.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner-django API. label Dec 3, 2020
@google-cla
Copy link

google-cla bot commented Dec 3, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 3, 2020
@mf2199
Copy link
Contributor Author

mf2199 commented Dec 3, 2020

@googlebot I consent.

@google-cla
Copy link

google-cla bot commented Dec 3, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tina80lvl
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 3, 2020
django_spanner/client.py Outdated Show resolved Hide resolved
django_spanner/expressions.py Outdated Show resolved Hide resolved
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.

The PR looks like a clear improvement (aside from the odd unicode chars), any reason it's still in draft?

@@ -123,6 +130,13 @@ def create_model(self, model):
self.create_model(field.remote_field.through)

def delete_model(self, model):
"""
Drop the model’s table in the database along with any unique constraints
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is a unicode RIGHT SINGLE QUOTATION MARK, did you write these docstrings in a rich text editor first?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed it.

@tina80lvl
Copy link
Contributor

The PR looks like a clear improvement (aside from the odd unicode chars), any reason it's still in draft?

I guess there is no such reason for now, I've fixed the rest changes.

@mf2199 mf2199 marked this pull request as ready for review December 10, 2020 15:27
@mf2199 mf2199 requested a review from a team as a code owner December 10, 2020 15:27
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.

Please fix the TODOs.

def runshell(self, parameters):
"""Override the base class method.

TODO: Consider actual implementation of this method.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear that we could implement this since cloud spanner doesn't have a "shell" executable. We might want to consider https://github.com/cloudspannerecosystem/spanner-cli or a similar project for this in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the docstring here, just raising NotSupportedError is fine since spanner doesn't really give us a way to support this.

Transform a time value to an object compatible with what is expected
by the backend driver for time columns.

:type value: #TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are still a lot of TODOs in the docstrings in this file.

Comment on lines 254 to 255
# TODO: here and below, remove unused expression and connection parameters
# or include them to the code.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mf2199 I moved this comment in d36b9ab since the preceding change had this comment in each of the converter docstrings:

#TODO remove unused expression and connection parameters or include them to the code.

But converters are part of the django db API, and we can't change the signature without also replacing django internals. See e.g. https://github.com/django/django/blob/8093aaa8ff9dd7386a069c6eb49fcc1c5980c033/django/db/models/sql/compiler.py#L1084.

def convert_binaryfield_value(self, value, expression, connection):
"""Convert a binary field to Cloud Spanner.
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT these docstrings are backwards: value comes from python-spanner, and we're converting from one bytes (or "bytes-like object") it into another: from a base64-encoded bytes returned by spanner to a non-base64-encoded bytes for django, since that's the internal type for django BinaryFields.

"""Convert a date and time field to Cloud Spanner.

:type value: #TODO
:param value: A binary field value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy/paste?

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. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants