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

#376 Implemented TryFrom<&str> for ObjectId #378

Closed

Conversation

clarkmcc
Copy link

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());

@abr-egn abr-egn self-assigned this Oct 12, 2022
@abr-egn
Copy link
Contributor

abr-egn commented Oct 12, 2022

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.

@clarkmcc
Copy link
Author

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?

@abr-egn
Copy link
Contributor

abr-egn commented Oct 13, 2022

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.

Implemented TryFrom<String> for ObjectId

Removed unused import
@clarkmcc clarkmcc force-pushed the 376-string-conversions-for-objectid branch from 4630a51 to 461402c Compare October 13, 2022 15:14
@abr-egn
Copy link
Contributor

abr-egn commented Oct 17, 2022

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

@clarkmcc
Copy link
Author

clarkmcc commented Nov 4, 2022

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.

@abr-egn
Copy link
Contributor

abr-egn commented Nov 7, 2022

No worries! Glad you found a way forward.

@abr-egn abr-egn closed this Nov 7, 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

Successfully merging this pull request may close these issues.

RUST-1508 Implement conversion traits between strings and ObjectId
2 participants