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.

Sign up for GitHub

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

Jump to bottom

Adding remaining HappyBase table helpers. #1536

Merged
merged 1 commit into from
Feb 25, 2016
Merged

Adding remaining HappyBase table helpers. #1536

merged 1 commit into from
Feb 25, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 25, 2016

No description provided.

@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Feb 25, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 25, 2016
theacodes
theacodes reviewed Feb 25, 2016
View reviewed changes
gcloud/bigtable/happybase/table.py
@@ -26,10 +26,18 @@
from gcloud.bigtable.column_family import GCRuleIntersection
from gcloud.bigtable.column_family import MaxAgeGCRule
from gcloud.bigtable.column_family import MaxVersionsGCRule
from gcloud.bigtable.happybase.batch import _get_column_pairs

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@theacodes
Copy link
Contributor

LGTM

@dhermes
Copy link
Contributor Author

dhermes commented Feb 25, 2016

I'm merging with the LGTM but am still interested and your answer to my bigtable._helpers question.

dhermes added a commit that referenced this pull request Feb 25, 2016
@dhermes
Merge pull request #1536 from dhermes/happybase-final-table-helpers
e225636 
Adding remaining HappyBase table helpers.
@dhermes dhermes merged commit e225636 into googleapis:master Feb 25, 2016
@dhermes dhermes deleted the happybase-final-table-helpers branch February 25, 2016 23:21
@theacodes
Copy link
Contributor

In general it's just a question of where you draw the "API boundary" - at the module level or at the package level. If you draw it at the package level, then what we're doing now is fine. If you draw it at the module level, then importing private members from other modules is a no-no, because you're overstepping the boundary. IMO, if a module's private function is useful outside of that module it should either be public or moved into a shared dependency.

/opinion /rant

@theacodes theacodes mentioned this pull request Mar 4, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@theacodes theacodes

Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhermes @theacodes @googlebot