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

Value::try_into makes use of TryInto difficult #162

Closed
djmitche opened this issue Dec 30, 2020 · 10 comments
Closed

Value::try_into makes use of TryInto difficult #162

djmitche opened this issue Dec 30, 2020 · 10 comments
Milestone

Comments

@djmitche
Copy link

I'd like to write some conversions for my own structs from instances of Value, and std::convert::TryFrom / TryInto are good choices for this. However, the existence of a try_into method on the struct, unrelated to these traits, shadows the trait method.

Could the try_into method be renamed into try_deserialize or something of the sort?

@matthiasbeyer
Copy link
Member

IMO, the try_from function should be reimplemented using the TryFrom trait anyways.

I think this is a good idea!

@matthiasbeyer
Copy link
Member

@conradludgate
Copy link
Contributor

Sadly I don't think it's possible to implement it using TryFrom for all T because it conflicts.

I'm not quite sure what with, but I think it's because a type could implement From<Value> and Deserialize individually downstream and that would have conflicting implementations

@conradludgate
Copy link
Contributor

Should we just go with try_deserialize() for now?

@djmitche
Copy link
Author

that sounds good to me :)

I'm confused, though, because it looks like @matthiasbeyer already did this?

@conradludgate
Copy link
Contributor

I'm confused, though, because it looks like @matthiasbeyer already did this?

Looking through their old maint fork, they only had this
https://git.sr.ht/~matthiasbeyer/config-rs-maint/tree/master/item/src/value.rs#L154. No TryFrom that I can find

@matthiasbeyer
Copy link
Member

Yeah I even tried to implement the TryInto trait, but it was not possible when I tried... But having a function named try_into() is counter-intuitive, because it suggestes that the trait is there... which it is not. So I guess try_deserialize() it is, right?

@conradludgate
Copy link
Contributor

I would say that's a consensus reached

@danieleades
Copy link
Contributor

looks like this is already merged. can this be closed?

@matthiasbeyer
Copy link
Member

Yes! Thanks for pinging me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants