Skip to content

RUST-754 Accept impl Borrow<T> in insert and replace methods #331

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

Merged

Conversation

patrickfreed
Copy link
Contributor

RUST-754

This PR updates the various insert and replace methods to accept impl Borrow<T>, which means that both owned values and values that can be converted into references can be accepted in these methods.

In a related change, the trait requirements on T in the sync driver were relaxed now that tokio and async-std's blocking functions no longer require it.

@@ -541,7 +541,7 @@ async fn empty_insert() {
.database(function_name!())
.collection::<Document>(function_name!());
match coll
.insert_many(Vec::new(), None)
.insert_many(Vec::<Document>::new(), None)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In certain cases, this will make type inference more difficult and require the usage of type arguments, such as in this case. I think this is worth it over taking in just &T though, which rules out owned values and other smart-pointer types from being passed in (e.g. Arc, Box). I think in most cases users won't be relying on the concrete type in these methods and the annotations won't be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this seems like a fairly similar situation to the one I outlined during the Serde work where the concrete type is needed in the method parameter to assign types to generic function calls like Vec::new. Our consensus was that a situation like that would be quite rare, and since we're doing a 2.0 I think it's fine to break this/require these types in the future (especially given that the only place this shows up within our testing is on this call which results in an error anyway).

@patrickfreed patrickfreed marked this pull request as ready for review May 4, 2021 00:44
@@ -541,7 +541,7 @@ async fn empty_insert() {
.database(function_name!())
.collection::<Document>(function_name!());
match coll
.insert_many(Vec::new(), None)
.insert_many(Vec::<Document>::new(), None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this seems like a fairly similar situation to the one I outlined during the Serde work where the concrete type is needed in the method parameter to assign types to generic function calls like Vec::new. Our consensus was that a situation like that would be quite rare, and since we're doing a 2.0 I think it's fine to break this/require these types in the future (especially given that the only place this shows up within our testing is on this call which results in an error anyway).

@patrickfreed patrickfreed merged commit bb945e7 into mongodb:master May 5, 2021
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.

2 participants