-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add new "quickstart" samples. #77
Conversation
@remi These 9 quickstart samples are *done, and are being pulled into docs from this branch. I'd love to write tests, but can you or @frankyn please help out on this front? I'm off writing these samples for 6 other languages... *defined as: region tags, comments, variable names, method calls, printing output, etc. have been finalized and standardized across the quickstart samples for the other languages. |
I'll work on the tests needed for these quickstarts. |
@@ -9,6 +9,13 @@ analytics data warehouse. | |||
|
|||
## Samples | |||
|
|||
### Quickstart | |||
|
|||
''' |
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.
What are these single quotes for?
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.
That is a incorrectly typed symbol. (I meant to use an apostrophe). I since have removed them, because Remi and I decided not to have them in the README for now.
|
||
# Run quickstart | ||
expect { | ||
load File::expand_path("quickstart.rb") |
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.
I love this! Let's just fix these to be File.expand_path
sans the ::
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.
Should also specify the full path to the quickstart relative to the spec directory (otherwise this uses the current working directory). So:
load File.expand_path("../quickstart.rb", __dir__)
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.
Understood!
|
||
it "creates a new dataset" do | ||
# Initialize test objects | ||
gcloud_test_client = Google::Cloud.new ENV["GOOGLE_CLOUD_PROJECT"] |
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.
Small #nit - Align variable assignments
Alignment of variable assignments is a common Ruby idiom and it is particularly useful for improving the readability of our code samples. So let's try to be consistent and always align (when it makes sense to do so)
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.
Understood.
|
||
expect(bigquery_test_client.dataset "my_new_dataset").not_to be nil | ||
|
||
# Clean up |
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 won't run if any of the above expectations fail.
Any cleanup should be placed in an after
block to ensure that it runs.
Because this example ensures that it runs no existing dataset (does the deletion before it runs the snippet code), I vote to remove this cleanup. Dataset will be left behind, but it will always be deleted before the example runs to ensure that the test setup is correct.
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.
If someone outside of Google runs the test do they have to pay for the Bigquery dataset?
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.
Yes, users pay for all Google Cloud Platform usage
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.
Got it.
project_id = "YOUR_PROJECT_ID" | ||
|
||
# Instantiates a client | ||
gcloud = Google::Cloud.new project_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.
Because we already have a comment with the wording "Instantiates a client", I don't think we need to call the BigQuery client bigquery_client
. I really like the clarity this adds but no other samples use this working.
All samples on the client library website use bigquery
not bigquery_client
:
https://googlecloudplatform.github.io/google-cloud-ruby/#/docs/google-cloud/v0.20.1/google/cloud/bigquery
Also, we use bigquery
in our code samples and never use a _client
suffix:
https://github.com/GoogleCloudPlatform/ruby-docs-samples/blob/2016.09.29/bigquery/tables.rb#L24
Let's update the syntax of the quickstarts to look more idiomatic, like so:
# Instantiates a client
gcloud = Google::Cloud.new project_id
bigquery = gcloud.bigquery
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.
@jmdobry Is it critically important for every quickstart to have variables suffixed with _client
?
No other Ruby samples use this, including google-cloud-ruby quickstarts. I would like the quickstart code to be conventional with code seen everywhere else.
For example, google-cloud-ruby has a quickstart on its homepage at: https://googlecloudplatform.github.io/google-cloud-ruby/#/
require "google/cloud"
gcloud = Google::Cloud.new "my-todo-project-id",
"/path/to/keyfile.json"
datastore = gcloud.datastore
product = datastore.find "Product", 123
And all other google-cloud-ruby snippets follow the following template:
require "google/cloud"
gcloud = Google::Cloud.new
storage = gcloud.storage
all_buckets = storage.buckets
|
||
it "creates a new dataset" do | ||
# Initialize test objects | ||
gcloud_test_client = Google::Cloud.new ENV["GOOGLE_CLOUD_PROJECT"] |
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.
Let's change these to just gcloud
and bigquery
like is used everywhere else.
I liked how explicit gcloud_test_client
is when initially drafting these specs to be super clear and self-document how these specs work:
https://gist.github.com/remi/f3abdee163a20e56c4d678c10e088d09
But now that I'm going back and reading these... I'd like to use the simple, consistent gcloud
and project_name
variables for these)
#consistency #nit
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.
Got it!
"Saved Task: Buy milk\n" | ||
}.to_stdout | ||
|
||
expect(datastore_test_client.find(task_key_test_client)).not_to be nil |
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.
Let's check that the sampletask's description is "Buy milk" in addition to verifying that the entity now exists
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.
Thoughts...
it "creates a new entity" do
gcloud = Google::Cloud.new ENV["GOOGLE_CLOUD_PROJECT"]
datastore = gcloud.datastore
sampletask = datastore.find "Task", "sampletask1"
datastore.delete sampletask if sampletask
expect(datastore.find "Task", "sampletask1").to be nil
expect(Google::Cloud).to receive(:new).with("YOUR_PROJECT_ID").and_return gcloud
expect {
load File.expand_path("../quickstart.rb", __dir__)
}.to output {
"Saved Task: Buy milk\n"
}.to_stdout
sampletask = datastore.find "Task", "sampletask1"
expect(sampletask).not_to be nil
expect(sampletask["description"]).to eq "Buy milk"
end
"Text: Hello, world!\n"+ | ||
"Sentiment: 1.0, 0.6000000238418579\n" | ||
).to_stdout | ||
|
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.
Remove trailing space in example block
expect { | ||
load File::expand_path("quickstart.rb") | ||
}.to output( | ||
"Text: Hello, world!\n"+ |
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.
Add space before +
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
def run_quickstart |
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.
Remove method wrappers for samples, just wrap in [START...
and [END...
region comments
# [END logging_quickstart] | ||
end | ||
|
||
if __FILE__ == $PROGRAM_NAME |
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.
Remove along with wrapper methods
…ples into quickstarts Keeping it up to date :D
…try fetch - logging.entries filter: was not working without additional order: option - simplify log name to "quickstart_log_#{Time.now.to_i}" - always try to delete log after spec runs, allowing for NotFound exceptions - refactor entry lookup into a helper method
Refactor Logging Quickstart spec and fix by adding order clause to entry fetch
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.
Some very small syntax changes requested. Otherwise looks great!
- Remove one last instances of
_client
- Use snake_case and never camelCase
- Don't use #! shabang
|
||
# Instantiates a client | ||
gcloud = Google::Cloud.new project_id | ||
language_client = gcloud.language |
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.
language_client
--> language
@@ -0,0 +1,34 @@ | |||
#!/usr/bin/ruby |
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.
Remove shabang line
describe "Language Quickstart" do | ||
|
||
it "detect sentiment" do | ||
gcloud_test_client = Google::Cloud.new ENV["GOOGLE_CLOUD_PROJECT"] |
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.
gcloud_test_client
--> gloud
speech = gcloud.speech | ||
|
||
# The name of the audio file to transcribe | ||
fileName = "./audio_files/audio.raw" |
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.
fileName
--> file_name
vision = gcloud.vision | ||
|
||
# The name of the image file to annotate | ||
fileName = "./images/cat.jpg" |
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.
fileName
--> file_name
|
||
describe "Vision Quickstart" do | ||
|
||
it "label a cat image" do |
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 description doesn't flow like a proper sentence.
Maybe it "performs label detection on sample image file"
?
/cc @jmdobry |
These are intended to be simple examples that explicitly show:
Products covered:
For reference: