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

Use rustler's builtin serde support #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

filmor
Copy link

@filmor filmor commented Jul 6, 2024

No description provided.

@benhaney
Copy link
Owner

benhaney commented Jul 6, 2024

Oh awesome, I didn't know Rustler had integrated Serde support now! Although it does look like it inherited the weird behavior of serde_rustler that forced me fork it, where the :ok and :error atoms get serialized as the strings "Ok" and "Err", inconsistent with all other atoms (see issue #6). Some other minor issues are that this probably breaks Decimal encoding support (because I manually implemented that in my serde_rustler fork) though I don't have a test for it apparently, and changes some error tuple text (which I care much less about and mostly just needs updated tests).

I'd love to use Rustler's built-in Serde, but it would need to match Jsonrs's existing behavior so we aren't regressing a bunch of bugs and breaking compatibility.

@filmor
Copy link
Author

filmor commented Jul 6, 2024

Thank you, that is exactly the input I was looking for. Would you mind opening an issue for this on the Rustler repo?

@praveenperera
Copy link

praveenperera commented Jul 7, 2024

@filmor that would be a nice change, then I could get rid of these lines in fast_rss

https://github.com/avencera/fast_rss/blob/6fa58824398c6119ced024377e31be54c4fa28c0/lib/fast_rss.ex#L67

@benhaney
Copy link
Owner

The ok/error serialization is fixed in Rustler (rusterlium/rustler#639), so now this PR is just waiting on Rustler to add the ability to register custom struct deserializers.

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.

3 participants