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 Visit::visit_key + Visit::visit_value #65

Open
KodrAus opened this issue Oct 5, 2021 · 4 comments
Open

Consider Visit::visit_key + Visit::visit_value #65

KodrAus opened this issue Oct 5, 2021 · 4 comments

Comments

@KodrAus
Copy link

KodrAus commented Oct 5, 2021

The Visit trait has a visit_entry method that accepts a key-value pair. In cases where you’re bridging different APIs you might not be able to guarantee both the key and value are present at the same time to pass to this method. For instance, serde::SerializeMap treats entries as an optimization and requires serializers support receiving keys and values independently. We ended up adding these separate methods to the standard library’s fmt builders for this reason.

We could consider making visit_entry forward to some new visit_key and visit_value methods.

@carllerche
Copy link
Member

wdyt @taiki-e ?

@carllerche
Copy link
Member

@KodrAus can you provide an example of a case where the key & value are not available at the same time or when we would need this separation to bridge an API?

@KodrAus
Copy link
Author

KodrAus commented Oct 8, 2021

@carllerche Something I’ve had is when lazily parsing a value while only keeping a single token around at a time. That’s bespoke enough to reasonably suggest “not fit for purpose” for, but is something I’ve run into.

For bridging, we’ve currently got valuable -> serde implemented, but for the other way around, in order to generically bridge a SerializeMap into a Visit we can’t rely on serialize_entry, because its contract states you need to support keys and values independently

@taiki-e
Copy link
Member

taiki-e commented Oct 18, 2021

The initial idea of valuable-serde had ToSerde (Serializable) and FromSerde, and IIRC this was needed when implementing FromSerde.

In the end, only ToSerde (Serializable) was implemented, so it was deferred. However, I think this is one of the features needed to support "what serde supports", so I have no objection to adding this.

@taiki-e taiki-e removed this from the v0.1 - Initial release milestone Jan 4, 2022
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

3 participants