bson-rust icon indicating copy to clipboard operation
bson-rust copied to clipboard

#376 Implemented TryFrom<&str> for ObjectId

Open clarkmcc opened this issue 3 years ago • 3 comments

Fixes #376 and allows for things like this

fn handle<T>(key: T) where T: TryInto<ObjectId> {
    let id = key.try_into();
}

handle("000000000000000000000000");
handle(ObjectId::new());

clarkmcc avatar Oct 11 '22 17:10 clarkmcc

Hi! Thanks for the contribution - I've authorized an evergreen run, and assuming that passes (modulo known-flaky tests) this'll be good to merge 🙂

As a side note, I don't think I've seen an API accept a TryFrom impl as a parameter, that seems a little unusual, and also might run into issues with the error type.

abr-egn avatar Oct 12 '22 14:10 abr-egn

As a side note, I don't think I've seen an API accept a TryFrom impl as a parameter, that seems a little unusual, and also might run into issues with the error type.

@abr-egn if this is unusual, then I'm probably going about this in the wrong way. What I'd like to be able to do is create functions that accept both strings or ObjectIds and do the necessary conversions under-the-hood. Is there a better way to accomplish this?

clarkmcc avatar Oct 12 '22 16:10 clarkmcc

It looks like there's an unused import in the test code - fix that and this'll be good to merge!

What I'd like to be able to do is create functions that accept both strings or ObjectIds and do the necessary conversions under-the-hood. Is there a better way to accomplish this?

Using TryFrom is definitely the right way to do that specific thing - what I typically see is a more distinct separation of input parsing from other logic.

abr-egn avatar Oct 13 '22 14:10 abr-egn

It looks like it's still failing the lint check - please make sure to use rustfmt to format the files.

abr-egn avatar Oct 17 '22 18:10 abr-egn

Sorry for the delay. Per your advice, I've found probably a more idiomatic way to handle this problem. If you'd like to close this without merging, that is no problem with me.

clarkmcc avatar Nov 04 '22 15:11 clarkmcc

No worries! Glad you found a way forward.

abr-egn avatar Nov 07 '22 16:11 abr-egn