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

derive that forwards From/ToBytes to specific encoding #661

Closed
ModProg opened this issue Jan 17, 2024 · 4 comments · Fixed by #667
Closed

derive that forwards From/ToBytes to specific encoding #661

ModProg opened this issue Jan 17, 2024 · 4 comments · Fixed by #667
Labels
enhancement New feature or request

Comments

@ModProg
Copy link
Contributor

ModProg commented Jan 17, 2024

My idea was to add to extism_convert either derive macros for the different data formats, or a single Encoding derive macro, that specifies a specific one.

In:

#[derive(ToJson, FromJson)]
// or
#[derive(ToEncoding, FromEncoding)]
#[encoding(Json)]
struct Struct { /* some fields */ }

Out:

// ToJson
impl<'a> ToBytes<'a> for Struct {
  type Bytes = <Json<Self> as ToBytes<'a>>::Bytes;
  
  fn to_bytes(&self) -> Result<Self::Bytes, Error> {
    Json(self).to_bytes()
  }
}
// FromJson
impl FromBytesOwned for Struct {
  fn from_bytes_owned(date: &[u8]) -> Result<Self, Error> {
    Json::from_bytes_owned(data).map(|j| j.0)
  }
}

This allows using it directly as function arguments/return type, and it automatically has the correct encoding (e.g., when sharing code between host and plugin)

@zshipko
Copy link
Contributor

zshipko commented Jan 18, 2024

Good idea! This would definitely reduce the amount of boilerplate when only one encoding is ever used for a type.

@zshipko zshipko added the enhancement New feature or request label Jan 19, 2024
@ModProg
Copy link
Contributor Author

ModProg commented Jan 19, 2024

I'd be up to look into implementing this, what would be your preferred API:

// require serde::{Serialize, DeserializeOwned}
#[derive(FromJson, ToJson)]
#[derive(FromMsgpack, ToMsgpack)]
// prost::Message
#[derive(FromProst, ToProst)]
// require bytemuck::Pod
#[derive(FromRaw, ToRaw)]

or

#[derive(FromBytes, ToBytes}
// require serde::{Serialize, DeserializeOwned}
#[encoding(Json)]
#[encoding(Msgpack)]
// prost::Message
#[encoding(Prost)]
// require bytemuck::Pod
#[encoding(Raw)]

@ModProg
Copy link
Contributor Author

ModProg commented Jan 19, 2024

I would prefer the latter because I think it would enable us to create something quite flexible.

we could accept any generic tuple struct that implements ToBytes as argument to the encode attribute.

@zshipko
Copy link
Contributor

zshipko commented Jan 19, 2024

I agree, I think the second option is preferred, we would just need to make sure that only one encoding type is specified.

Thanks for picking this up - let me know if you run into any issues!

zshipko pushed a commit that referenced this issue Feb 5, 2024
closes #661.

- [x] docs
- [x] tests
- [x] depend on `extism-convert/extism-pdk-path` feature in
https://github.com/extism/rust-pdk
extism/rust-pdk#47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants