-
Notifications
You must be signed in to change notification settings - Fork 374
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
feat(spanner): add samples for proto columns #13759
Conversation
Also add "singer_cc_proto" dependencies to the tests that use it directly, as would be required if strict checking was enabled.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13759 +/- ##
==========================================
- Coverage 93.23% 93.15% -0.09%
==========================================
Files 2233 2233
Lines 193351 193517 +166
==========================================
- Hits 180265 180263 -2
- Misses 13086 13254 +168 ☔ View full report in Codecov by Sentry. |
Here is the summary of changes. You are about to add 7 region tags.
This comment is generated by snippet-bot.
|
@@ -989,6 +990,22 @@ void AddTimestampColumn( | |||
} | |||
// [END spanner_add_timestamp_column] | |||
|
|||
//! [drop-column] |
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.
comment: It looked suspicious that this tag is not referenced by a @snippet
. I assume the tag is there in case we Ctrl
+F
the drop-column
banner.
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 think(?) we have a few such non-devrel, non-snippet tags that are indeed only there to match their sub-command name, or otherwise delineate a block of code.
In this particular case it was only introduced so that I could adapt the pre-existing table-creation sample to the prerequisites of the proto-column samples. If I recall correctly, the Java samples have "setup" and "teardown" code for each sample so that they are all independent, but we've evolved a model where we try to execute them in (some) sequence, so occasionally some between-sample adaptation is necessary.
google::protobuf::FileDescriptorSet fds; | ||
google::cloud::spanner::testing::SingerInfo::default_instance() | ||
.GetMetadata() | ||
.descriptor->file() |
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 assume we can assume this is never nullptr
, or that if it is, something else has gone wrong.
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.
My recollection is that it can indeed never be null.
if (!commit_result) throw std::move(commit_result).status(); | ||
} | ||
std::cout << "Update was successful " | ||
<< "[spanner_update_data_with_proto_message_column]\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.
Here and below, is printing the [spanner_*]
intentional?
Is there logic to when we print it and when we don't?
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.
Intentional, yes. As you can see, many do, so I usually try to keep doing that (although now that we have SampleBanner("tag")
, it is probably less important). So, I'm afraid the only logic is: Is the "[tag]" even slightly helpful, and do you remember to add it.
void UpdateDataWithProtoMessageColumn(google::cloud::spanner::Client client) { | ||
google::cloud::spanner::testing::SingerInfo singer_proto; | ||
singer_proto.set_singer_id(2); | ||
singer_proto.set_birth_date("1942-06-18"); |
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.
🪲
Also add "singer_cc_proto" dependencies to the tests that use it directly, as would be required if strict checking was enabled.
This change is