-
Notifications
You must be signed in to change notification settings - Fork 8
chore: Convert JSON LDValue into the AiConfig Object #61
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
base: lc/SDK-1192/AI-config-feature-branch
Are you sure you want to change the base?
chore: Convert JSON LDValue into the AiConfig Object #61
Conversation
I just realized I also need to copy the .github file from another project in this mono-repo to have the test run in CI. I am running the test locally via |
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/datamodel/AiConfig.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
lib/sdk/server-ai/src/main/java/com/launchdarkly/sdk/server/ai/LDAiClient.java
Outdated
Show resolved
Hide resolved
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.
Suggestions in comments.
f442874
to
877d4e2
Compare
Co-Authored-By: lchan@launchdarkly.com <lchan@launchdarkly.com>
Co-Authored-By: lchan@launchdarkly.com <lchan@launchdarkly.com>
I think the only optional change I may make in this PR is to use the builder pattern for the AiConfig. The changes from Devin and me addressed Ryan's feedback, added more comprehensive unit tests and is ready for review. I will do the other methods (Actual AiConfig Feature Flag evaluation, LDAIConfigTracker) in separate PRs. |
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.
Add nullable and nonnull annotations on fields to help with consumption.
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
lib/sdk/server-ai/src/test/java/com/launchdarkly/sdk/server/ai/LDAiClientTest.java
Outdated
Show resolved
Hide resolved
|
||
protected <T> T ldValueNullCheck(T ldValue) throws AiConfigParseException { | ||
if (ldValue == LDValue.ofNull()) { | ||
throw new AiConfigParseException("Unexpected Null value for non-optional field"); |
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.
Probably helpful to add field name.
…ructors package-private Co-Authored-By: lchan@launchdarkly.com <lchan@launchdarkly.com>
Also, I realized using Ai is not to spec - in the future PR I will update everything to say AI instead. |
Generally speaking it would be using whatever is idiomatic to the language. Some languages have convenient standards. Like two letter initialisms are uppercase, but anything else follows the normal standard. So in C# Java isn't as concrete, but we should be consistent.
Generally we call this out in the spec. |
Convert JSON LDValue into the AiConfig Object
For ease of discussion, implement only the conversion logic from LDValue (that we will get back from the Feature Flag SDK) to the AiConfig DataModel.
This uses the exception-based short circuiting (similar to https://github.com/launchdarkly/dotnet-core/blob/6774539af1a2f87b96ae3e647fdcdc5663c791ab/pkgs/sdk/server-ai/src/LdAiClient.cs#L180) and LDValue reading logic similar to https://github.com/launchdarkly/python-server-sdk-ai/blob/main/ldai/client.py#L134
Updates
Link to Devin run
https://app.devin.ai/sessions/f1057ea9206643789c93d45aa8c5c500
Requested by: lchan@launchdarkly.com