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

Consider removing text::Encode in favour of serde::Serialize #97

Closed
nox opened this issue Sep 20, 2022 · 8 comments
Closed

Consider removing text::Encode in favour of serde::Serialize #97

nox opened this issue Sep 20, 2022 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@nox
Copy link
Contributor

nox commented Sep 20, 2022

Serde's Serialize trait is implemented for dozens of types in the Rust ecosystem, its derive code is fairly well optimised, and it is the most popular serialization framework we have for Rust.

What do you think of removing the Encode trait in favour of Serialize, to get all the nice things there are in Serde for free?

@mxinden
Copy link
Member

mxinden commented Sep 21, 2022

I would be in favor of that. I should have thought of it before.

@nox would you like to write a first proposal? Before you go all the way, I would suggest writing a small proof-of-concept first to make sure using serde is a viable path forward.

@mxinden mxinden added the help wanted Extra attention is needed label Sep 21, 2022
@nox
Copy link
Contributor Author

nox commented Sep 21, 2022

Sure, I'll write a struct Bridge<T>(T) that implements Encode using T's implementation of Serialize, in a separate crate.

@nox
Copy link
Contributor Author

nox commented Sep 23, 2022

I pushed https://github.com/nox/serde_prometheus_labels earlier today.

@nox
Copy link
Contributor Author

nox commented Oct 10, 2022

I published serde_prometheus_labels which provides its own Family<Labels, Metric> type using serde as the encoder.

@Victor-N-Suadicani
Copy link
Contributor

How would this interact with a problem like the one in #75? I mean, () also implements Serialize/Deserialize but you probably shouldn't be able to use () as a label, for instance. So I'm not sure Encode can be entirely replaced by serde's traits, as serde's traits are basically implemented for too many types that aren't suitable.

@nox
Copy link
Contributor Author

nox commented Oct 19, 2022

I mean, () also implements Serialize/Deserialize but you probably shouldn't be able to use () as a label, for instance.

Why not? () should mean "no labels at all".

@nox
Copy link
Contributor Author

nox commented Oct 19, 2022

So I'm not sure Encode can be entirely replaced by serde's traits, as serde's traits are basically implemented for too many types that aren't suitable.

I agree to this, but I would rather rely on serde and not be blocked on some missing implementation. It made the job for us, that's why I published that stuff as a separate crate.

@mxinden
Copy link
Member

mxinden commented Jan 2, 2023

With #105 I don't think we should proceed here, i.e. I don't see the value of replacing the new traits.

@nox I am closing here for now. Please comment here in case I am missing something.

The above said I do believe implementing EncodeLabelSet, EncodeLabel, EncodeLabelKey and EncodeLabelValue for more types in std would be helpful.

@mxinden mxinden closed this as completed Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants