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

Update push and pull docs #2685

Merged
merged 13 commits into from Dec 4, 2023
Merged

Update push and pull docs #2685

merged 13 commits into from Dec 4, 2023

Conversation

hans-thomas
Copy link
Contributor

Fixes #2387,
Hello there, I updated the push and pull methods documentation to solve the confusion about using these methods.

@hans-thomas hans-thomas changed the base branch from 4.1 to 4.0 November 23, 2023 13:45
@hans-thomas hans-thomas changed the base branch from 4.0 to 4.1 November 23, 2023 13:45
@hans-thomas hans-thomas marked this pull request as ready for review November 23, 2023 13:46
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

I like this improvement. It's a good step for more clarity.

docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
@GromNaN GromNaN added this to the 4.1 milestone Nov 24, 2023
docs/query-builder.md Outdated Show resolved Hide resolved
@GromNaN GromNaN requested a review from jmikola November 27, 2023 09:06
docs/query-builder.md Outdated Show resolved Hide resolved
]);
```

If you **DON'T** want duplicate items, set the third parameter to `true`:
If you **DON'T** want duplicate values, set the third parameter to `true`:
Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked the code, but this suggests that the boolean causes $addToSet to be used internally instead of $push. Is that correct?

Having a boolean argument control that rubs me the wrong way (I'll add it to the growing list), but I assume there's an explanation for this behavior. Is the push() method here actually derived from something else in Eloquent, or was it originally created in this library specifically for the MQL operators?

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 haven't checked the code, but this suggests that the boolean causes $addToSet to be used internally instead of $push. Is that correct?

Yes. It's the logged queries of pushing normally and pushing unique values.

array:2 [
  0 => array:3 [
    "query" => "users.updateMany({"_id":"65672f58551357588a07c993"},{"$push":{"items":{"$each":["boots"]}}},{"multiple":true})"
    "bindings" => []
    "time" => 1.82
  ]
  1 => array:3 [
    "query" => "users.updateMany({"_id":"65672f58551357588a07c993"},{"$addToSet":{"items":{"$each":["hat","jeans"]}}},{"multiple":true})"
    "bindings" => []
    "time" => 0.6
  ]
]

Having a boolean argument control that rubs me the wrong way (I'll add it to the growing list), but I assume there's an explanation for this behavior. Is the push() method here actually derived from something else in Eloquent, or was it originally created in this library specifically for the MQL operators?

The push method is originally from Eloquent and it's used for saving a model instance and its relationships to the database. this library overrides the push method and accepts three args. If no arg is passed, the original push method will be called.

Copy link
Member

@jmikola jmikola Nov 29, 2023

Choose a reason for hiding this comment

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

Is Eloquent's push method documented anywhere beyond Recursively Saving Models & Relationships? That doesn't appear to take any arguments and the method seems entirely different in purpose (this doesn't have to do with relationships).

@GromNaN: A boolean flag to determine whether $push or $addToSet is used seems like a poor design as it makes the code less self-documenting. I think this is compounded by the fact that the method on the model class uses func_get_args() and doesn't define a named parameter that users could specify if they so desired.

It may be worth making a ticket to revisit this down the line. Perhaps introduce a new addToSet() method and then deprecate the original variation. This might also be a moot point when/if the query builder is integrated.

Copy link
Member

Choose a reason for hiding this comment

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

💯 agree that the double behavior of this method is a bad design.

If deprecate the custom behavior of Model::push with parameters, we have to do deprecate the pull method too.

Let's improve the documentation for now.

docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
Co-Authored-By: Jeremy Mikola <jmikola@gmail.com>
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Outdated Show resolved Hide resolved
docs/query-builder.md Show resolved Hide resolved
Co-Authored-By: Jeremy Mikola <jmikola@gmail.com>
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM, so over to @GromNaN.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Thank you @hans-thomas for the care taken with the documentation and for your kind review @jmikola

@GromNaN GromNaN merged commit 24c3592 into mongodb:4.1 Dec 4, 2023
3 checks passed
@hans-thomas hans-thomas deleted the 2387-update-docs branch December 5, 2023 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants