Skip to content
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

feat: add LDValue_SerializeJSON C binding #458

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

cwaldren-ld
Copy link
Contributor

@cwaldren-ld cwaldren-ld commented Oct 17, 2024

This adds a new API to the LDValue interface for serializing to a JSON string.

Example:

LDValue foo = LDValue_NewBool(true);
char *json = LDValue_SerializeJSON(foo);
// json is the string 'true'
LDMemory_FreeString(json);
LDValue_Free(foo);

It's easier to view this PR commit to commit. The first one was necessary in order to access the boost::json tag_invoke implementations in common, where LDValue is implemented. Luckily, I only needed to pull over the value headers and some utilities.

One thing to note is that boost serializes numbers using scientific notation (e.g. 17 serializes to 1.7E1). Kind of an odd choice, but it is to JSON spec.

BEGIN_COMMIT_OVERRIDE
refactor: move json_errors, primitives, and value from internal to common lib
feat: add LDValue_SerializeJSON C binding
END_COMMIT_OVERRIDE

@cwaldren-ld cwaldren-ld force-pushed the cw/sdk-779-ldvalue-serialize branch 4 times, most recently from 340afa6 to f9cdb11 Compare October 17, 2024 01:50
@@ -1,6 +1,5 @@
#pragma once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This header wasn't necessary.

@cwaldren-ld cwaldren-ld marked this pull request as ready for review October 17, 2024 01:51
@cwaldren-ld cwaldren-ld requested a review from a team as a code owner October 17, 2024 01:51
Copy link
Member

@kinyoklion kinyoklion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned about the ultimate destination of the header moves. Specifically I am concerned that it may make boost a part of our public API. I am down with providing serialization methods, but not with exposing the serialization implementation.

@cwaldren-ld
Copy link
Contributor Author

@kinyoklion, I've moved the serialization headers to /detail/serialization in the common lib.

@cwaldren-ld cwaldren-ld force-pushed the cw/sdk-779-ldvalue-serialize branch from 598010e to 5881d05 Compare October 28, 2024 18:28
@cwaldren-ld cwaldren-ld merged commit 90eb880 into main Oct 30, 2024
23 checks passed
@cwaldren-ld cwaldren-ld deleted the cw/sdk-779-ldvalue-serialize branch October 30, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants