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

add ujson bridge #401

Merged
merged 8 commits into from
Jan 22, 2024
Merged

add ujson bridge #401

merged 8 commits into from
Jan 22, 2024

Conversation

esarbe
Copy link
Contributor

@esarbe esarbe commented Jan 18, 2024

Add a ujson/upickle implementation

@satabin
Copy link
Member

satabin commented Jan 19, 2024

Hi @esarbe. Thanks for the contribution, new library support is always awesome.

Can you please run sbt prePR and push the result to make the CI happy?

case "replace" =>
val path = readPointer(value)
val payload =
try value.obj("value")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a way with ujson to check for existence before failing instead of try/catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, there's the .get(key) method that behaves pretty much like in maps.

But we'll end up throwing exceptions anyway, it's the idiomatic way to propagate error in ujson/upickle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewrote the bridge without try/catch.

Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with ujson but if that's the idiomatic way, then it's ok.

@satabin satabin added this to the 4.6.0 milestone Jan 19, 2024
@esarbe
Copy link
Contributor Author

esarbe commented Jan 19, 2024

Applied prePR. It's a bit eager though and also overrides the copyright header.

Fixed it and pushed.

@satabin
Copy link
Member

satabin commented Jan 19, 2024

Applied prePR. It's a bit eager though and also overrides the copyright header.

Fixed it and pushed.

The license header is part of the check indeed, the year and authors are configured in the build and checked by the CI, so build won't pass if they do not match the configuration.

@esarbe
Copy link
Contributor Author

esarbe commented Jan 20, 2024

Updated the build.

build.sbt Outdated Show resolved Hide resolved
@esarbe
Copy link
Contributor Author

esarbe commented Jan 22, 2024

Checks fail at binary compatibility checks, which is not surprising, since there are no previous binaries to compare to.

I can change this, but I don't know what your preferred approach to this "new library bridge" problem is.

@satabin
Copy link
Member

satabin commented Jan 22, 2024

Checks fail at binary compatibility checks, which is not surprising, since there are no previous binaries to compare to.

I can change this, but I don't know what your preferred approach to this "new library bridge" problem is.

In this case, you can use the tlVersionIntroduced setting for the module. And this will be in version 4.6.0. For instance, you can look at this module setting.

@satabin satabin enabled auto-merge January 22, 2024 20:18
@satabin satabin added this pull request to the merge queue Jan 22, 2024
Merged via the queue into gnieh:main with commit c429203 Jan 22, 2024
12 checks passed
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