google-cloud-cpp icon indicating copy to clipboard operation
google-cloud-cpp copied to clipboard

feat(spanner): add spanner::Value support for TypeCode::UUID

Open devbww opened this issue 9 months ago • 6 comments

This change is Reviewable

devbww avatar Apr 09 '25 21:04 devbww

/gcbrun

scotthart avatar Apr 10 '25 17:04 scotthart

Codecov Report

:x: Patch coverage is 96.29630% with 2 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 92.92%. Comparing base (7595793) to head (967c7cb). :warning: Report is 161 commits behind head on main.

Files with missing lines Patch % Lines
google/cloud/spanner/value.cc 87.50% 2 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15076   +/-   ##
=======================================
  Coverage   92.92%   92.92%           
=======================================
  Files        2389     2389           
  Lines      214832   214886   +54     
=======================================
+ Hits       199631   199693   +62     
+ Misses      15201    15193    -8     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Apr 10 '25 18:04 codecov[bot]

Thanks again for the PR.

While the protos defining the UUID field are public, service support for the type isn't live yet. We'll merge this PR and write some integration tests when UUID is available in production.

scotthart avatar Apr 10 '25 19:04 scotthart

While the protos defining the UUID field are public, service support for the type isn't live yet. We'll merge this PR and write some integration tests when UUID is available in production.

Okie dokie.

devbww avatar Apr 10 '25 23:04 devbww

/gcbrun

scotthart avatar Apr 28 '25 14:04 scotthart

While the protos defining the UUID field are public, service support for the type isn't live yet. We'll merge this PR and write some integration tests when UUID is available in production.

I would proffer that this should be merged irrespective of the rollout of service support. It is the server support that actually matters, not the conversion between types within the client.

Also, merging now would mean that there is no need to wait for a new client release when server support is enabled. And that means client-side development can start early, including writing the integration tests, which can also actually be run against the staging service.

Finally, it avoids the possibility of future merge conflicts.

devbww avatar May 06 '25 21:05 devbww