feat: resolve ambiguous ServiceAccountConfig constructors (#15886)#16076
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ServiceAccountConfig class to use the C++ type system for constructor disambiguation, replacing the previous approach that used multiple absl::optional parameters. New structs, ServiceAccountFilePath and ServiceAccountJsonObject, have been introduced to wrap the respective string values. The feedback focuses on adhering to the Google C++ Style Guide by adding descriptive comments to the new structs and replacing a redundant comment in the header file with more meaningful documentation.
| Options options_; | ||
| }; | ||
|
|
||
| struct ServiceAccountFilePath { |
There was a problem hiding this comment.
The Google C++ Style Guide requires a comment for every class and struct describing its purpose. This struct is used to disambiguate the ServiceAccountConfig constructors.
/// A wrapper for a service account file path.
struct ServiceAccountFilePath {References
- Every class should have a comment that describes what it is and what it should be used for. (link)
| explicit ServiceAccountFilePath(std::string p) : value(std::move(p)) {} | ||
| }; | ||
|
|
||
| struct ServiceAccountJsonObject { |
There was a problem hiding this comment.
The Google C++ Style Guide requires a comment for every class and struct describing its purpose. This struct is used to disambiguate the ServiceAccountConfig constructors.
/// A wrapper for a service account JSON object string.
struct ServiceAccountJsonObject {References
- Every class should have a comment that describes what it is and what it should be used for. (link)
| // enforces this comment. | ||
| ServiceAccountConfig(absl::optional<std::string> json_object, | ||
| absl::optional<std::string> file_path, Options opts); | ||
| // Just declarations ending with a semicolon |
There was a problem hiding this comment.
This comment is redundant and does not provide useful information about the class's contract or behavior. Consider removing it or replacing it with a meaningful description of the constructors.
/// Constructs a configuration from a service account file path.
antonjfernando2021
force-pushed
the
feature/Make_better_constructors_ServiceAccountConfig_15886
branch
from
0876509 to
6ea8fc0
Compare
|
"I've replaced the ambiguous ServiceAccountConfig constructor with two explicit overloads using new ServiceAccountFilePath and ServiceAccountJsonObject structs, as requested in #15886. This intentionally changes the API to enforce correct usage at compile time." |
1 participant
Description
ServiceAccountFilePathandServiceAccountJsonObjectstructs.Closes #15886