-
Notifications
You must be signed in to change notification settings - Fork 3
feat: incomplete C bindings for client configuration #45
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
Conversation
|
This pull request has been linked to Shortcut Story #201468: Create C bindings for Config type. |
|
|
||
| LD_EXPORT(LDClientConfigBuilder) | ||
| LDClientConfigBuilder_New(char const* sdk_key) { | ||
| return FROM_BUILDER(new ConfigBuilder(sdk_key ? sdk_key : "")); |
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.
I believe as of c++2023 it will be illegal to even pass nullptr to an std::string constructor.
|
|
||
| return LDStatus_Success(); | ||
| } | ||
|
|
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.
I made this a helper so we can ensure builders are handled consistently across the C binding (like - always assigning nullptr to out result on failure, or whatever we choose.)
I do wonder this method should consume the builder, though. It's not consuming in C++, so you could theoretically inspect the error and allow for rebuilding until the problem is fixed - but I'm not sure that would be a common use-case.
68b85d9 to
19d8e4e
Compare
This PR is mainly to determine if the binding style I'm using here will make sense, it's not a full implementation. In the spirit of not having massive PRs.
Missing are all the various sub-builders.
The main idea is that we'll have 4 opaque types,
LD(Client|Server)Config(Builder). No naming collisions, so someone could use both SDKs in the same app.But it does imply we'd need to dupe all the builder methods for both SDKs.
I feel like this makes sense, since we might have different config options for each. Otherwise, we would need to document specific methods / arrange the includes such that you can't accidentally call
LDConfigSetSomeClientThingon a server config.Ryan you might have better ideas.
Finally, I've added in an
LDStatustype. On the C++ side we usetl::expected<T, Error>. In C, we can mirror this pattern with out params and return value: