-
Notifications
You must be signed in to change notification settings - Fork 10
Conversation
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.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devjgm)
CHANGELOG.md, line 8 at r1 (raw file):
**BREAKING CHANGES:** * refactor!: remove grpc_utils namespace; left aliases for backward compat (#158)
I think just this message is not enough context, maybe cut&paste the commit message (with some edits) into the release notes?
**POTENTIALLY BREAKING CHANGE:** Refactor the types and functions from
`google::cloud::grpc_utils` to `google::cloud::`. The old header files
and types should continue to work, as we kept aliases for them, but
there is some risk we missed something. The library name (the physical
.a and/or .so file) is not changed, the target names for CMake are not
changed. For Bazel, the old targets continue to work, but you might want
to move to newer targets that do not expose the backwards compatibility
headers.
Up to you.
Yes, you're right. Of course. Updated. PTAL |
Codecov Report
@@ Coverage Diff @@
## master #172 +/- ##
=======================================
Coverage 96.22% 96.22%
=======================================
Files 94 94
Lines 4395 4395
=======================================
Hits 4229 4229
Misses 166 166 Continue to review full report at Codecov.
|
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.
Additional nits on the release notes, but up to you.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devjgm)
CHANGELOG.md, line 14 at r2 (raw file):
is not changed, the target names for CMake are not changed, the target name for Bazel moved to the new location.
That was true for the original commit, but no longer true after the additional changes for backwards compatibility, thus my edits. Consider:
... for CMake are not changed. For Bazel, the old targets continue to work, but
you might want to move to newer targets that do not expose the backwards
compatibility headers.
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @coryan)
CHANGELOG.md, line 14 at r2 (raw file):
for CMake are not changed. For Bazel, the old targets continue to work, but
you might want to move to newer targets that do not expose the backwards
compatibility headers
Sigh. I need to read more closely. Sorry. Accepted your edits.
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.
Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
This change is