-
Notifications
You must be signed in to change notification settings - Fork 64
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
fix(update/4): improve Mongo.update/4 function #245
Conversation
- prevent endless recursion when supplied empty list - make options list optional - add more documentation + tests
defp normalise_updates(updates), do: normalise_updates([updates]) | ||
defp normalise_updates([{_, _} | _] = updates), do: normalise_updates([updates]) | ||
|
||
defp normalise_updates(updates), do: updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only normalise Keyword lists, letting Mongo return an error if the updates are not in an expected format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some names for the placeholders to increase the readability? For example:
Instead of
defp normalise_updates([[{_, _} | _] | _] = updates) do
it would be nice to have
## maps lists of keywords list
defp normalise_updates([[{_key, _value} | _rest] | _updates] = updates) do
@@ -1129,7 +1129,29 @@ defmodule Mongo do | |||
|
|||
e.g. long-hand `query` becomes short-hand `q`, snake case `array_filters` | |||
becomes `arrayFilters` | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make a mention of some of the usual options (upsert & multi) ? I was a bit surprised they couldn't be combined, but it makes sense after some thought.
Mongo.update(MongoPool, "test_collection", [ | ||
[q: %{foo: 24}, update: %{flag: "old"}], | ||
[q: %{foo: 99}, update: %{luftballons: "yes"}, upsert: true] | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these examples suffice? I think this shows the most common ways of supplying updates, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :-) Maybe you want to add some comments to the private functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Thanks for the review, I added variable names and comments for readability. I don't have write access, so can you merge it when you have time. |
Changes:
Fixes #243