refactor!: replaced Row<...> with std::tuple<...> #967
Conversation
googlebot
added
the
cla: yes
There was a problem hiding this comment.
Chore or Refactor? Up to you... Couple of items below, reverting the changes to the README.md file is a blocker in my opinion.
Reviewed 16 of 16 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devjgm)
google/cloud/spanner/README.md, line 24 at r1 (raw file):
* **Breaking Changes** * refactor `Read` to return `ReadResult`; remove `ResultSet` (#935) * removed `std::tuple<>` from mutations API (#938). Removes the `AddRow(std::tuple<Ts...>)`
This is probably a search&replace error, the release notes for previous versions should not change.
google/cloud/spanner/integration_tests/client_integration_test.cc, line 622 at r1 (raw file):
EXPECT_STATUS_OK(row); if (!row) break; std::vector<Value> v;
FYI, this pattern to initialize a std::vector<Value> repeats 3 times here, should we refactor it?
devjgm
changed the title
Codecov Report
@@ Coverage Diff @@
## master #967 +/- ##
==========================================
- Coverage 94.5% 94.06% -0.45%
==========================================
Files 94 158 +64
Lines 3951 10294 +6343
==========================================
+ Hits 3734 9683 +5949
- Misses 217 611 +394
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Good point. I think refactor: is better. Changed.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @coryan)
google/cloud/spanner/README.md, line 24 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
This is probably a search&replace error, the release notes for previous versions should not change.
Oops! reverted. Yes, search-n-replace issue.
google/cloud/spanner/integration_tests/client_integration_test.cc, line 622 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
FYI, this pattern to initialize a
std::vector<Value>repeats 3 times here, should we refactor it?
Yeah, I noticed and thought about this too. Here are the relevant trade offs that I considered.
-
Each of these three cases happen to use a tuple with the exact same type. A
std::tuple<int, string, string>. So we could write a helper function that accepts that specific type and returns avector<Value>. But if any of these integration tests change the tuple type, the helper would no longer work. -
If we instead wrote a helper function that generically works for any
std::tuple<...>, the helper function gets much more complicated. Probably more complicated than makes sense in a _test.cc file. -
We could just repeat the few lines in each place because it's probably what end-users would actually write, and it's easy to change if the tuple elements change.
Basically, option 2 didn't sound good to me. The helper would be more complicated than repeating these lines a few times.
Option 1 would be OK with me.
As you can see, I went with option 3, but I'm happy to go with option 1 if you prefer that one. Or an option 4 that I didn't consider.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
google/cloud/spanner/integration_tests/client_integration_test.cc, line 622 at r1 (raw file):
Previously, devjgm (Greg Miller) wrote…
Yeah, I noticed and thought about this too. Here are the relevant trade offs that I considered.
Each of these three cases happen to use a tuple with the exact same type. A
std::tuple<int, string, string>. So we could write a helper function that accepts that specific type and returns avector<Value>. But if any of these integration tests change the tuple type, the helper would no longer work.If we instead wrote a helper function that generically works for any
std::tuple<...>, the helper function gets much more complicated. Probably more complicated than makes sense in a _test.cc file.We could just repeat the few lines in each place because it's probably what end-users would actually write, and it's easy to change if the tuple elements change.
Basically, option 2 didn't sound good to me. The helper would be more complicated than repeating these lines a few times.
Option 1 would be OK with me.
As you can see, I went with option 3, but I'm happy to go with option 1 if you prefer that one. Or an option 4 that I didn't consider.
SGTM.
There was a problem hiding this comment.
Oops. Didn't see that tidy warning. Changed to emplace_back to quiet clang-tidy. I'll let the CI builds pass before submitting.
Reviewable status: 14 of 15 files reviewed, all discussions resolved (waiting on @coryan)
…cloud-cpp-spanner#967) This is a prefactoring (pre-refactoring) in preparatoin for googleapis/google-cloud-cpp-spanner#387, which will create a new non-templated Row class. This PR makes room for that new class, and leave most of the code looking like it will ultimately look: When a user wants a typed-Row, they will specify the row's type using std::tuple. BREAKING CHANGE: Removes the Row<...> type.
This is a prefactoring (pre-refactoring) in preparatoin for #387, which will create a new non-templated
Rowclass. This PR makes room for that new class, and leave most of the code looking like it will ultimately look: When a user wants a typed-Row, they will specify the row's type usingstd::tuple.BREAKING CHANGE: Removes the
Row<...>type.This change is