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

Support for String<Bytes> or similar #392

Open
vorner opened this issue Nov 15, 2020 · 13 comments
Open

Support for String<Bytes> or similar #392

vorner opened this issue Nov 15, 2020 · 13 comments

Comments

@vorner
Copy link
Contributor

vorner commented Nov 15, 2020

Hello

Thanks for #31, I can't wait for a release 😇.

Anyway, it would be nice if strings could get similar support for referencing the Bytes buffer too.

I don't have a preferred/thought through solution, only some ideas how that could work:

  • Picking a crate that does this kind of wrapping of Bytes into something derefing to &str (there were some mentioned in optimize for protos containing large blobs  #31, I don't know how well maintained they are), maybe make the dependency optional. Then the same option would change String -> String<Bytes> as the one that currently changes Vec<u8> -> Bytes.
  • Allow the user to configure type replacements on fields, eg. saying that field_type_replace("String", "Whatever", &["paths"]). That type would have to support Message already in some way, but that would be up to the user. That could probably be used to replace „nested“ fields encoded as bytes by the actual thing inside.

But anything that works would be nice :-).

Thank you

@vorner
Copy link
Contributor Author

vorner commented Jan 15, 2021

Thinking about it more, that field_type_replace approach would be more flexible, as it would also allow using things like smallstring or smallvec at places, to optimize things.

❓ If I tried writing the support for prost-build, would there be interest in accepting it (after figuring out proper naming, what to do with generics in the Vec -> SmallVec case, etc), or would that be considered out of scope for the crate?

@danburkert
Copy link
Collaborator

Yes I'd like to support this, in fact a recent refactor in danburkert@79f0dfd was laying the groundwork. I don't think the set of types you will be able to substitue will be able to be 'open', because prost-derive is going to have to know how to encode/decode them. So I'd like to have an API which allows you to select from an enum, with String/Bytes/Secret variants.

@danburkert
Copy link
Collaborator

secret from #369

@vorner
Copy link
Contributor Author

vorner commented Jan 16, 2021

About being open… I was wondering if there would be a trait these things need to satisfy (it probably isn't prost::Message, is it?). For Vec for repeated fields, it could possibly be something like Extend, what to do about string or bytes fields ‒ I'm not sure there's an existing trait describing that one.

Right now I'm looking for a good String(Bytes) thing on crates.io and deciding if there is one or if I want to roll my own (it would be nice if such string thing could have auxiliary functions like split or lines that return these owned String(Bytes) instead of &str, for example). But I would probably not expect that to be directly supported by prost and it would still be nice to be able to somehow plug it in ‒ be it by having the right trait, or duck-typing (it generates code, having a same-named method would be enough)

(Yes, if the string field is represented as Bytes, I can wrap it into such type manually, so it's not a huge deal, but it would still be convenient to be supported out of the box, without manual post-processing).

@vorner
Copy link
Contributor Author

vorner commented Jan 31, 2021

If anyone is interested in longer and more verbose description of what I have in mind: https://vorner.github.io/2021/01/31/saving-some-allocations.html

If you'd be interested in doing it with the traits as I mentioned above, I get my hands dirty and see if I can come up with a reasonable PR for that.

@Kixunil
Copy link

Kixunil commented Dec 17, 2021

I want this feature too. @vorner are you still working on it?
I'd also like field_encoding_replace that instead of Message implemented on type uses a custom module. This would be similar to #[serde(with = "some::module")]. I noticed integers are already implemented as modules.

@Kixunil
Copy link

Kixunil commented Dec 17, 2021

I randomly noticed there's a substitutions branch - it seems to implement this. Maybe just resolve the conflict and merge?

@vorner
Copy link
Contributor Author

vorner commented Dec 18, 2021

are you still working on it?

Not actively. I have that bytes-utils crate around, and I'm intent on keeping it alive, but otherwise don't have any actual plans or any work in progress around it. Besides, I currently don't have the need for that feature any more (but I still think it would be a neat thing).

@Kixunil
Copy link

Kixunil commented Dec 18, 2021

OK, will try look into making it myself.

@rustonaut
Copy link

Is anyone currently working on this?

I have some use case for the generic version for supporting url::Url fields (through in
difference to the other examples here that would be a fail-able parsing 🤔 ).

Depending on how much work it is I might(1) be able to contribute it.

(1): I will most likely know weather or not I could contribute, and how much time I could spend on it later this week.

@LucioFranco
Copy link
Member

No one is currently working on this at the moment.

@rustonaut
Copy link

rustonaut commented May 23, 2022 via email

@Kixunil
Copy link

Kixunil commented Jun 17, 2022

@rustonaut I did spend some time on it but it's more complex than I expected, so couldn't finish it.

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

No branches or pull requests

5 participants