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

Import of serde_rustler #586

Merged
merged 15 commits into from
Mar 20, 2024
Merged

Import of serde_rustler #586

merged 15 commits into from
Mar 20, 2024

Conversation

filmor
Copy link
Member

@filmor filmor commented Jan 8, 2024

Hi all, please have a look. This started as a direct import of https://github.com/sunny-g/serde_rustler/ into our codebase (@sunny-g, thanks again for your work!).

Changes

The simple example code now looks like this:

#[nif]
pub fn readme(SerdeTerm(animal): SerdeTerm<Animal>) -> SerdeTerm<Animal> /* or `impl Encoder` */ {
    println!("\n deserialized animal from README example: {:?}", animal);
    SerdeTerm(animal)
}

As far as I can see, it's not feasible to completely replace our Encoder/Decoder construction as serde does not allow for a "passthrough" (i.e. we allow a Term to be "encoded" to Term and vice-versa) and can also not be extended to do that.

Issues

  • The encoding table is very "elixir-y" right now (note to self: there is still a bit of code where I tried to change that, haven't completed it)
  • License (see below)
  • Documentation
  • More tests?

License

@sunny-g Your project is missing a license, we'd need one from you to proceed with this. If you don't really care, the simplest would probably to donate your code to the project, but this has to be explicit. If you put it under MIT/Apache license, we'd have to carry your copyright along.

@filmor filmor force-pushed the serde branch 3 times, most recently from 4f9b5d1 to 8a25089 Compare January 9, 2024 10:05
@filmor filmor requested a review from a team January 9, 2024 10:06
@filmor
Copy link
Member Author

filmor commented Jan 9, 2024

The CI errors are the same that we have in #581 right now

@filmor
Copy link
Member Author

filmor commented Jan 20, 2024

On the license: The crates.io says it's MIT, which is a subset of Apache 2, so we should be fine importing it. The source code doesn't contain license notes or a license file, so there is nothing to retain. Our MIT license note is quite out-dated anyhow, so I will adjust it as follows (provided everyone is fine with it):

  Copyright (c) 2016 hansihe
+ Copyright (c) 2016-2024 The Contributors of the Rustler Project
+ Serde support derived from `serde_rustler` Copyright (c) 2019-2021 sunny-g

@sunny-g Please comment if

  1. This is fine with you
  2. You want your full name instead of your Github handle
  3. You are fine with leaving the explicit copyright out in favour of a mention in the Changelog or Readme

@rusterlium/core Please have a look at the approach, it would be nice to get some comments/feedback as this is quite a large chunk of (at least slightly) opionated code to be added to our codebase.

@sunny-g
Copy link
Contributor

sunny-g commented Jan 20, 2024

@filmor sorry for the delay

just updated the repo to include an MIT LICENSE file, and either name ref is fine by me, though I guess it should be my name since that is what's in the LICENSE file.

Let me know what else you need - glad to see this getting first-party support!

Copy link
Member

@evnu evnu left a comment

Choose a reason for hiding this comment

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

Great work! I only found minor nitpicks (but it is a lot of code, so we might find something else later on). Thanks for your work @sunny-g and @filmor !

rustler/src/serde/de.rs Outdated Show resolved Hide resolved
rustler/src/serde/mod.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,112 @@
encode_jobs = %{
Copy link
Member

Choose a reason for hiding this comment

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

Probably not for now, but maybe later: We should move the benchmarks to rustler_benchmarks.

@sunny-g
Copy link
Contributor

sunny-g commented Jan 22, 2024

Great work! I only found minor nitpicks (but it is a lot of code, so we might find something else later on). Thanks for your work @sunny-g and @filmor !

it was my first rust project, and a ton's changed since I wrote it, so be kind :)

@filmor filmor marked this pull request as ready for review March 20, 2024 17:30
@filmor filmor changed the title Initial import of serde_rustler Import of serde_rustler Mar 20, 2024
@filmor filmor merged commit 3e39581 into master Mar 20, 2024
10 of 14 checks passed
@filmor filmor deleted the serde branch March 20, 2024 17:33
leandrocp added a commit to leandrocp/mdex that referenced this pull request Apr 17, 2024
leandrocp added a commit to leandrocp/mdex that referenced this pull request Apr 17, 2024
* Replace serde_rustler with rustler::serde

Ref rusterlium/rustler#586
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