Skip to content

RUST-1858 Add builders for bulk write models #1104

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
merged 1 commit into from
May 16, 2024

Conversation

isabelatkinson
Copy link
Contributor

This also has the changes from #1103, filtering by the last commit will show the relevant changes for this PR.

Comment on lines +22 to +25
let mut models_vec = Vec::new();
for model in models.into_iter() {
models_vec.push(model.into());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think models.into_iter().map(Into::into).collect() will do two passes, did a manual impl here to be safe

@@ -32,66 +36,205 @@ pub struct BulkWriteOptions {
#[serde(untagged)]
#[non_exhaustive]
pub enum WriteModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypedBuilder doesn't work for enums with directly-embedded fields

@isabelatkinson isabelatkinson marked this pull request as ready for review May 15, 2024 22:45
@isabelatkinson isabelatkinson requested a review from abr-egn May 15, 2024 22:45
@@ -31,136 +38,22 @@ impl<'de> Deserialize<'de> for WriteModel {
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
enum WriteModelHelper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this impl still needed? This looks equivalent to what I think #[derive(Deserialize)] on WriteModel would produce now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this limitation still holds unfortunately, the test files use a "tagged" representation whereas the ops array doesn't

Copy link
Contributor

Choose a reason for hiding this comment

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

Derp, right.

@isabelatkinson isabelatkinson merged commit d895a6d into mongodb:main May 16, 2024
6 of 8 checks passed
@isabelatkinson isabelatkinson deleted the model-builders branch May 17, 2024 14:13
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