-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
bigtable: add raw gcloud-python hello sample. #384
Conversation
@lesv @jonparrott please take a look. |
'Hello World!', | ||
'Hello Cloud Bigtable!', | ||
'Hello Python!', | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra newline between expressions and control statements, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I hadn't heard that rule before. Seems like this check should be automated and made part of the linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/shruggie
It's more a personal nit of mine than something that's been codified.
LGTM once you've done Jon's comments. Mine will need to happen next week. |
This sample uses the "raw" [gcloud-python Cloud Bigtable package](https://googlecloudplatform.github.io/gcloud-python/stable/bigtable-usage.html).
from gcloud import bigtable | ||
|
||
|
||
def main(project_id, cluster_id, zone, table_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not saying this needs to block the merge, but I think it's better to separate these actions into separate functions. 1) because smaller methods are better 2) because it makes it easier to define a CLI , jon suggested it for the logging samples which you can see here, which to me makes the sample more interactive/usable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I the sound of that. Right now the sample is kind of boring, though it does show the basic operations.
There is a Go command-line program for Bigtable, https://godoc.org/google.golang.org/cloud/bigtable/cmd/cbt, though that covers more operations than are probably necessary for a "hello world" app, but I do think it makes sense to do something similar here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another benefit (or maybe downside?) of splitting it into functions and adding a command-line interface is it would almost force us into handling errors properly.
For example, I know an exception is thrown if the table already exists, but with putting everything in main()
I thought it would clutter it up too much to catch that exception. Having it in a function can make it easier to show how to handle those errors and explain them in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably okay for a hello world app, you can write a separate crud sample if you really want to.
I'm ambivalent about error handling - I think as long as our samples don't silence errors and propagate them to the user then that is enough for a sample.
This sample uses the "raw" gcloud-python Cloud Bigtable
package.