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

Adding Bigtable Cell class. #1473

Merged
merged 2 commits into from
Feb 16, 2016
Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 15, 2016

Used to hold data in rows retrieved.

@tseaver This module could definitely use some design review (none of the content here yet, but worth discussing now or at least thinking about).

Right now, the implementation is as follows:

  • Rows are read from a Table (and a Cell is like a "column" in a row, so there can be many), so the helpers needed to parse a row that was read are in row_data.py
  • Mutations (in rows) are created directly on a Row, and this is all done in row.py (all code submitted or merged)

Used to held data in rows retrieved.
@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Feb 15, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 15, 2016
@tseaver
Copy link
Contributor

tseaver commented Feb 16, 2016

Travis failed on the docs build:

Found undocumented public modules:

- gcloud.bigtable.row_data

ERROR: InvocationError: '/home/travis/build/GoogleCloudPlatform/gcloud-python/.tox/docs/bin/python /home/travis/build/GoogleCloudPlatform/gcloud-python/scripts/verify_included_modules.py'

@tseaver
Copy link
Contributor

tseaver commented Feb 16, 2016

No explicit tests for the Cell class' methods?

@dhermes
Copy link
Contributor Author

dhermes commented Feb 16, 2016

Double fail. Sorry, forgot to stage it. Must've been in a hurry.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 16, 2016

@tseaver PTAL.

Still would like to know what you think about having row.py and row_data.py separately.

@tseaver
Copy link
Contributor

tseaver commented Feb 16, 2016

Still would like to know what you think about having row.py and row_data.py separately.

My only quibble is with the filename: given that the module just contains the Cell class, ISTM that it would most logically be named gcloud.bigtable.cell.

@dhermes
Copy link
Contributor Author

dhermes commented Feb 16, 2016

@tseaver
Copy link
Contributor

tseaver commented Feb 16, 2016

Cell is just the beginning:

SGTM

@dhermes
Copy link
Contributor Author

dhermes commented Feb 16, 2016

And the content of this commit?

@tseaver
Copy link
Contributor

tseaver commented Feb 16, 2016

Fine to merge.

dhermes added a commit that referenced this pull request Feb 16, 2016
@dhermes dhermes merged commit 180db64 into googleapis:master Feb 16, 2016
@dhermes dhermes deleted the bigtable-cell branch February 16, 2016 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable 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