-
Notifications
You must be signed in to change notification settings - Fork 441
doc(v3): update top level docs #15956
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(v3): update top level docs #15956
Conversation
Summary of ChangesHello @scotthart, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on updating the project's documentation and build system configurations to align with newer minimum version requirements for essential tools and third-party libraries. It reflects a shift to C++17 as the baseline standard and streamlines information by removing outdated deprecation notices and consolidating platform support details. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request updates documentation and configuration files for the next major version. The changes include bumping minimum versions for dependencies like CMake, Bazel, and compilers, and updating the required C++ standard to C++17. Several documentation files are updated to reflect these changes, including removing obsolete deprecation notices. My review found a couple of minor inconsistencies in the documentation that could be improved for clarity. Overall, the changes look good and are consistent with a major version release.
doc/compile-time-configuration.md
Outdated
| The default Bazel toolchain forces C++17 on Linux and macOS. `google-cloud-cpp` | ||
| requires C++17, so you will need to update the C++ standard version. You must |
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 sentence is a bit confusing. It states that the default toolchain and the library both require C++17, but then says an update is needed. Suggest rephrasing for clarity.
| The default Bazel toolchain forces C++17 on Linux and macOS. `google-cloud-cpp` | |
| requires C++17, so you will need to update the C++ standard version. You must | |
| `google-cloud-cpp` requires C++17. You must ensure your Bazel toolchain is | |
| configured to use C++17 or a newer standard. You must either: |
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.
Reworded.
| | [OpenSSL][openssl-gh] | 3.0.17 | Crypto functions for [^3] | | ||
| | [nlohmann/json][nlohmann-json-gh] | 3.12.0 | JSON for Modern C++ [^3] | | ||
| | [protobuf][protobuf-gh] | 6.33.x | Protobuf is needed for any library based on gRPC [^5] [^6] | | ||
| | [OpenTelemetry][opentelemetry-gh] | 1.23.0 | An observability framework[^7] | |
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.
With OpenTelemetry now being a required dependency, the referenced footnote [^7] is outdated as it still refers to it as an 'optional dependency'. Suggest removing the reference to avoid confusion.
| | [OpenTelemetry][opentelemetry-gh] | 1.23.0 | An observability framework[^7] | | |
| | [OpenTelemetry][opentelemetry-gh] | 1.23.0 | An observability framework | |
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.
Updated footnote.
642f856 to
56958cf
Compare
diegomarquezp
left a comment
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.
LGTM, just one qq
| notice. These include `google/cloud/internal/`, and | ||
| `google/cloud/testing_utils/`. | ||
|
|
||
| ## Supported Platforms |
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.
qq: Why is this section being removed? Is 3.x compatible with all common platforms?
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.
That same information already exists in the top level README.md. It was duplicated here and didn't seem necessary to keep both.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## prepare-for-v3.0.0 #15956 +/- ##
===================================================
Coverage 92.61% 92.62%
===================================================
Files 2332 2332
Lines 213322 213322
===================================================
+ Hits 197574 197587 +13
+ Misses 15748 15735 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0c61457
into
googleapis:prepare-for-v3.0.0
This change is