-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1338 +/- ##
==========================================
+ Coverage 94.64% 94.66% +0.02%
==========================================
Files 172 174 +2
Lines 13615 13629 +14
==========================================
+ Hits 12886 12902 +16
+ Misses 729 727 -2
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.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @devjgm)
README.md, line 3 at r1 (raw file):
# Google Cloud Spanner C++ Client Library <!-- This file is automatically generated by ci/test-readme/generate-readme.sh -->
I think we can stop automatically generating this file now?
README.md, line 5 at r1 (raw file):
<!-- This file is automatically generated by ci/test-readme/generate-readme.sh --> Cloud Spanner is the only enterprise-grade, globally-distributed, and
I think including a /full/ description of the service was a DPE requirement for FA, please check go/api-client-quality specially the GA section about the GitHub README.
README.md, line 20 at r1 (raw file):
## Status This library supports Cloud Spanner at the [Beta](README.md#versioning) quality
I think we should say something, because we will always have libraries "not GA" and users will wonder "Is this GA?" if we don't say so explicitly. But it can be a brief bullet point:
- This library is GA and ready for production workloads. We have not plans to change the API.
Also: we should have a document describing what we mean by "change the API" (e.g. exclude internal
namespace, exclude testing
namespace, we reserve the right to change a struct/class to an alias, etc), but that is for a future PR.
README.md, line 33 at r1 (raw file):
* Windows, macOS, Linux * Most reasonable compiler versions, even back to gcc 4.9 and clang 3.8
Why not say:
- C++11 (and higher) compilers. We test with GCC >= 4.9, Clang >= 3.8, and MSVC >= 2019.
nit: I think the spelling is GCC and Clang.
README.md, line 36 at r1 (raw file):
* Environments with or without exceptions * Bazel and CMake builds * \>= C++11
This seems redundant, but if we are going to say it, use words:
- This library targets C++11 and higher. We do not plan to support C++98 or C++03.
ci/test-readme/generate-readme.sh, line 24 at r1 (raw file):
# Google Cloud Spanner C++ Client Library <!-- This file is automatically generated by ci/test-readme/$(basename "$0") -->
Consider removing this 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.
This is a major improvement in clarity, thanks! Some comments below, most of which are discussions starters than really hard blockers.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @devjgm)
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.
Thanks.
Reviewable status: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @coryan)
README.md, line 3 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I think we can stop automatically generating this file now?
It still needs to inject the contents of quickstart.cc
, which is why I kept this script. But that's the only thing. If GitHub markdown would let me include the contents of a file, I'd happily switch to that. Otherwise, I don't know of any other options.
README.md, line 5 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I think including a /full/ description of the service was a DPE requirement for FA, please check go/api-client-quality specially the GA section about the GitHub README.
The line I see is "A full description of the API", but I assume that's talking about the Client library, not the server. It would seem silly contain a full description of Spanner's server API.
FWIW, the Go spanner library and Java spanner library both only use a single sentence (or less) to describe spanner. I was following Java's lead here.
README.md, line 20 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I think we should say something, because we will always have libraries "not GA" and users will wonder "Is this GA?" if we don't say so explicitly. But it can be a brief bullet point:
- This library is GA and ready for production workloads. We have not plans to change the API.
Also: we should have a document describing what we mean by "change the API" (e.g. exclude
internal
namespace, excludetesting
namespace, we reserve the right to change a struct/class to an alias, etc), but that is for a future PR.
I can add that line, but first here's why I removed it FTR
AFAICT, none of our other libraries say that (that I could find). Much like with cloud services, I think we generally say nothing when a service is "GA", and instead label it "preview" or "beta" in those rare cases. I was thinking we could do the same here.
README.md, line 33 at r1 (raw file):
C++11 (and higher) compilers. We test with GCC >= 4.9, Clang >= 3.8, and MSVC >= 201
LGTM. Done.
README.md, line 36 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
This seems redundant, but if we are going to say it, use words:
- This library targets C++11 and higher. We do not plan to support C++98 or C++03.
Since I added the C++11 (and higher) compilers. We test with GCC >= 4.9, Clang >= 3.8, and MSVC >= 2019
line, I think you're right that this is redundant. Removed it.
ci/test-readme/generate-readme.sh, line 24 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Consider removing this file.
I think because of the use of sed -n
to include the quickstart.cc, I need to keep it.
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 2 of 2 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
README.md, line 5 at r1 (raw file):
Previously, devjgm (Greg Miller) wrote…
The line I see is "A full description of the API", but I assume that's talking about the Client library, not the server. It would seem silly contain a full description of Spanner's server API.
FWIW, the Go spanner library and Java spanner library both only use a single sentence (or less) to describe spanner. I was following Java's lead here.
SGTM
Part of #1310
You can preview these changes by looking at my fork: https://github.com/devjgm/google-cloud-cpp-spanner/tree/new-readme
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)