Conversation
|
I'm wondering why these snippets are to live in this repo and not as top-level godoc examples in the library code itself. This comment is likely specific to Go, and something we're having ongoing discussions with @rakyll and the eng team (including @jba). A few high level comments:
I will comment on specific code/files once the above is done. |
Whether you eventually want https://cloud.google.com to source samples from I'll work through the rest of your comments. |
|
Yeah, perfect. Don't worry too much about the directory name, then. If you prefer, I (or Jaana) can also just take it from here and put it in the appropriate place, and add you as a reviewer. |
Done
Done
Done
Done
Done
Done |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
CLAs look good, thanks! |
jmdobry
force-pushed
the
quickstarts
branch
2 times, most recently
from
bc64334 to
2dcff97
Compare
bigquery/bigquery_quickstart/main.go
Outdated
| "golang.org/x/net/context" | ||
| "log" | ||
|
|
||
| // Imports the Google Cloud Datastore client package |
There was a problem hiding this comment.
We follow the typical Go comment style to keep these types of guidance comments as proper sentences.
// Imports the Google Cloud Datastore client package.
bigquery/bigquery_quickstart/main.go
Outdated
| // license that can be found in the LICENSE file. | ||
|
|
||
| // [START bigquery_quickstart] | ||
| // Sample bigquery_quickstart creates a Google BigQuery dataset. |
There was a problem hiding this comment.
It is not very common to name packages with an underscore in Go. Can we call name these as bigquery-quickstart?
bigquery/bigquery_quickstart/main.go
Outdated
| // The name for the new dataset | ||
| datasetName := "my_new_dataset" | ||
|
|
||
| // Prepares the new dataset |
There was a problem hiding this comment.
Maybe we should just say "Create a dataset instance." if we need to explain the line.
bigquery/bigquery_quickstart/main.go
Outdated
| log.Fatalf("Failed to create dataset: %v", err) | ||
| } | ||
|
|
||
| fmt.Printf("Dataset %v created", dataset) |
There was a problem hiding this comment.
Dataset doesn't have a nice Stringer. I'd just leave this as fmt.Println("Dataset created.")
bigquery/bigquery_quickstart/main.go
Outdated
| log.Fatalf("Failed to create dataset: %v", err) | ||
| } | ||
|
|
||
| fmt.Printf("Dataset %v created", dataset) |
There was a problem hiding this comment.
Missing new line at the end.
| // Use of this source code is governed by the Apache 2.0 | ||
| // license that can be found in the LICENSE file. | ||
|
|
||
| // [START vision_quickstart] |
vision/vision_quickstart/main.go
Outdated
|
|
||
| import ( | ||
| "fmt" | ||
| "golang.org/x/net/context" |
There was a problem hiding this comment.
move context to another group.
vision/vision_quickstart/main.go
Outdated
| } | ||
|
|
||
| // The name of the image file to annotate | ||
| fileName := "vision/testdata/cat.jpg" |
There was a problem hiding this comment.
s/fileName/filename. See https://en.wikipedia.org/wiki/Filename.
| log.Fatalf("Failed to create image: %v", err) | ||
| } | ||
|
|
||
| labels, err := client.DetectLabels(ctx, image, 10) |
vision/vision_quickstart/main.go
Outdated
|
|
||
| labels, err := client.DetectLabels(ctx, image, 10) | ||
|
|
||
| fmt.Printf("Labels:\n") |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
CLAs look good, thanks! |
jmdobry
force-pushed
the
quickstarts
branch
3 times, most recently
from
e0fd9d8 to
e34cb4b
Compare
|
@waprin moving to e-mail |
The prefix instructions for queries are incorrect in the Google Cloud Storage sample according to my testing. The `/` belongs at the end of the prefix to look up files in a directory, not at the beginning.
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
4 participants
These are intended to be simple examples that explicitly show:
Note: This is my first attempt at writing Go, please be nice 😄
Note 2: The variable names, region tags, structure of the sample, etc. were all carefully chosen and currently match the "quickstart" samples for other languages (linked to below), so this is how we'd like the samples to look. But, if I've done something glaringly non-idiomatic then let's fix that.
Note 3: I'd love to have provided tests, but can you (golang-samples maintainers) help out with the tests? I'm off writing these samples in 6 other languages... I ran all of the samples locally to make sure they work.
Products covered:
For reference, see the corresponding samples in other languages:
masterin variousquickstart.jsfiles)Thanks!