Skip to content
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

doc(bigtable): deprecate RowReader public ctors #8887

Merged

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented May 5, 2022

The deprecation part of #8854


This change is Reviewable

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label May 5, 2022
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: c327d1db088d0f9ad76008922b50e1c902f3d9ae

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #8887 (07da1d1) into main (13e5b01) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8887      +/-   ##
==========================================
- Coverage   93.16%   93.16%   -0.01%     
==========================================
  Files        1477     1477              
  Lines      124810   124836      +26     
==========================================
+ Hits       116283   116302      +19     
- Misses       8527     8534       +7     
Impacted Files Coverage Δ
google/cloud/bigtable/row_reader.cc 26.31% <ø> (-73.69%) ⬇️
google/cloud/bigtable/version.h 100.00% <ø> (ø)
google/cloud/bigtable/row_reader.h 100.00% <100.00%> (ø)
google/cloud/bigtable/row_reader_test.cc 100.00% <100.00%> (ø)
google/cloud/bigtable/table.cc 99.25% <100.00%> (+0.01%) ⬆️
google/cloud/bigtable/async_read_stream_test.cc 97.99% <0.00%> (+0.66%) ⬆️
google/cloud/pubsub/subscriber_connection_test.cc 97.91% <0.00%> (+0.69%) ⬆️
...cloud/pubsub/internal/subscription_session_test.cc 98.75% <0.00%> (+0.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13e5b01...07da1d1. Read the comment docs.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 07da1d1acaee22ad18e75985de33adfe63a5e270

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc dbolduc marked this pull request as ready for review May 5, 2022 22:46
@dbolduc dbolduc requested a review from a team as a code owner May 5, 2022 22:46
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot improve on your choice of tag (doc:) but it seems weird.

@@ -51,13 +61,15 @@ class RowReader {
// NOLINTNEXTLINE(readability-identifier-naming)
static std::int64_t constexpr NO_ROWS_LIMIT = 0;

GOOGLE_CLOUD_CPP_BIGTABLE_ROW_READER_CTOR_DEPRECATED()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this change, the lack of default constructor is annoying. It makes it impossible to write code like this:

RowReader reader;
if (condition) {
  reader = table.ReadRows(...);
} else {
  reader = table.ReadRows(... /* but slightly different dots */);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #8892

@dbolduc
Copy link
Member Author

dbolduc commented May 6, 2022

I cannot improve on your choice of tag (doc:) but it seems weird.

That is probably a sign that I should have split this up into two PRs.

  • a refactor: that introduces bigtable_internal::MakeRowReader()
  • a doc: that adds compiler warnings, CHANGELOG entry, etc.

@dbolduc dbolduc merged commit 248740a into googleapis:main May 6, 2022
@dbolduc dbolduc deleted the bigtable-deprecate-row-reader-ctors branch May 6, 2022 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants