chore(gae): migrate region tags from projectionqueries.go #5108
Conversation
product-auto-label
bot
added
the
samples
![code-review-assist[bot]](https://avatars.githubusercontent.com/in/1008254?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @OremGLG and reviewers,
I'm Gemini, and I'm currently reviewing this pull request. In the meantime, I'm providing a summary of the changes for you and other reviewers to quickly get up to speed.
This pull request, "chore(gae): migrate region tags from projectionqueries.go", aims to migrate region tags by adding the prefix gae_datastore to regions within the file docs/appengine/datastore/projectionqueries/projectionqueries.go. This addresses several issues: b/392146363, b/392145933, b/392146306, b/391185644, and b/391186679.
Here's a summary of the changes:
- File Modified:
docs/appengine/datastore/projectionqueries/projectionqueries.go - Changes: The primary change involves adding
gae_datastoreas a prefix to several code comments. Specifically, the comments that previously started with// [START using_1],// [START using_2],// [START grouping],// [START projections_and_multiple_valued_properties_1], and// [START projections_and_multiple_valued_properties_2]now have thegae_datastoreprefix added, resulting in comments like// [START gae_datastore_using_1],// [START gae_datastore_using_2], etc. This is consistent across the[START]and[END]markers for each section.
For example, lines 31-35 changed from:
31: // [START using_1]
32: q := datastore.NewQuery("People").Project("FirstName", "LastName")
33: // [END using_1]
34: _ = q
35:}
to:
31: // [START gae_datastore_using_1]
32: // [START using_1]
33: q := datastore.NewQuery("People").Project("FirstName", "LastName")
34: // [END using_1]
35: // [END gae_datastore_using_1]
Similar changes were made for other sections in the file.
I'll be providing a more detailed review shortly.
A little code-review haiku for your enjoyment:
Prefixes neatly placed,
Clarity shines, bugs disappear,
Code's song now sings true.
There was a problem hiding this comment.
This pull request migrates region tags by adding the "gae_datastore" prefix to the regions in projectionqueries.go. This change clarifies the region tags and improves consistency. The changes are well-implemented and straightforward. The updated region tags accurately reflect the code snippets they enclose.
I have the following minor suggestions based on the Effective Go guidelines:
| // [START gae_datastore_using_1] | ||
| // [START using_1] | ||
| q := datastore.NewQuery("People").Project("FirstName", "LastName") | ||
| // [END using_1] | ||
| // [END gae_datastore_using_1] |
There was a problem hiding this comment.
Can we consolidate the start and end tags for gae_datastore_using_1 and using_1 into a single tag? This would reduce redundancy and improve readability. For example:
// [START gae_datastore_using_1]
q := datastore.NewQuery("People").Project("FirstName", "LastName")
// [END gae_datastore_using_1]| // [START gae_datastore_using_1] | |
| // [START using_1] | |
| q := datastore.NewQuery("People").Project("FirstName", "LastName") | |
| // [END using_1] | |
| // [END gae_datastore_using_1] | |
| // [START gae_datastore_using_1] | |
| q := datastore.NewQuery("People").Project("FirstName", "LastName") | |
| // [END gae_datastore_using_1] |
| // [START gae_grouping_grouping] | ||
| // [START grouping] | ||
| q := datastore.NewQuery("Person"). | ||
| Project("LastName", "Height").Distinct(). | ||
| Filter("Height >", 20). | ||
| Order("-Height").Order("LastName") | ||
| // [END grouping] | ||
| // [END gae_grouping_grouping] |
There was a problem hiding this comment.
The region tags gae_grouping_grouping and grouping seem redundant. It's best to avoid redundant tags and use a single, descriptive tag. Consider using just gae_datastore_grouping to ensure consistency with other examples. For example:
// [START gae_datastore_grouping]
q := datastore.NewQuery("Person").
Project("LastName", "Height").Distinct().
Filter("Height >", 20).
Order("-Height").Order("LastName")
// [END gae_datastore_grouping]| // [START gae_grouping_grouping] | |
| // [START grouping] | |
| q := datastore.NewQuery("Person"). | |
| Project("LastName", "Height").Distinct(). | |
| Filter("Height >", 20). | |
| Order("-Height").Order("LastName") | |
| // [END grouping] | |
| // [END gae_grouping_grouping] | |
| // [START gae_datastore_grouping] | |
| q := datastore.NewQuery("Person"). | |
| Project("LastName", "Height").Distinct(). | |
| Filter("Height >", 20). | |
| Order("-Height").Order("LastName") | |
| // [END gae_datastore_grouping] |
| // [START gae_datastore_projections_and_multiple_valued_properties_1] | ||
| // [START projections_and_multiple_valued_properties_1] | ||
| entity := Foo{A: []int{1, 1, 2, 3}, B: []string{"x", "y", "x"}} | ||
| // [END projections_and_multiple_valued_properties_1] | ||
| // [END gae_datastore_projections_and_multiple_valued_properties_1] |
There was a problem hiding this comment.
Can we consolidate the region tags here as well, similar to the previous suggestions? Using a single tag gae_datastore_projections_and_multiple_valued_properties_1 would improve consistency and reduce redundancy. For example:
// [START gae_datastore_projections_and_multiple_valued_properties_1]
entity := Foo{A: []int{1, 1, 2, 3}, B: []string{"x", "y", "x"}}
// [END gae_datastore_projections_and_multiple_valued_properties_1]| // [START gae_datastore_projections_and_multiple_valued_properties_1] | |
| // [START projections_and_multiple_valued_properties_1] | |
| entity := Foo{A: []int{1, 1, 2, 3}, B: []string{"x", "y", "x"}} | |
| // [END projections_and_multiple_valued_properties_1] | |
| // [END gae_datastore_projections_and_multiple_valued_properties_1] | |
| // [START gae_datastore_projections_and_multiple_valued_properties_1] | |
| entity := Foo{A: []int{1, 1, 2, 3}, B: []string{"x", "y", "x"}} | |
| // [END gae_datastore_projections_and_multiple_valued_properties_1] |
| // [START gae_datastore_projections_and_multiple_valued_properties_2] | ||
| // [START projections_and_multiple_valued_properties_2] | ||
| q := datastore.NewQuery("Foo").Project("A", "B").Filter("A <", 3) | ||
| // [END projections_and_multiple_valued_properties_2] | ||
| // [END gae_datastore_projections_and_multiple_valued_properties_2] |
There was a problem hiding this comment.
For consistency, let's consolidate the tags gae_datastore_projections_and_multiple_valued_properties_2 and projections_and_multiple_valued_properties_2 into a single tag gae_datastore_projections_and_multiple_valued_properties_2. For example:
// [START gae_datastore_projections_and_multiple_valued_properties_2]
q := datastore.NewQuery("Foo").Project("A", "B").Filter("A <", 3)
// [END gae_datastore_projections_and_multiple_valued_properties_2]| // [START gae_datastore_projections_and_multiple_valued_properties_2] | |
| // [START projections_and_multiple_valued_properties_2] | |
| q := datastore.NewQuery("Foo").Project("A", "B").Filter("A <", 3) | |
| // [END projections_and_multiple_valued_properties_2] | |
| // [END gae_datastore_projections_and_multiple_valued_properties_2] | |
| // [START gae_datastore_projections_and_multiple_valued_properties_2] | |
| q := datastore.NewQuery("Foo").Project("A", "B").Filter("A <", 3) | |
| // [END gae_datastore_projections_and_multiple_valued_properties_2] |
|
Here is the summary of changes. You are about to add 5 region tags.
This comment is generated by snippet-bot.
|
OremGLG
deleted the
garciadiaz-chore-migrate-region-projection-queries
branch
OremGLG
restored the
garciadiaz-chore-migrate-region-projection-queries
branch
Description
Migrate regions by adding "gae_datastore" prefix to regions from file docs/appengine/datastore/projectionqueries/projectionqueries.go
Fixes
b/392146363
b/392145933
b/392146306
b/391185644
b/391186679
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
go test -v ./..(see Testing)gofmt(see Formatting)go vet(see Formatting)