-
-
Notifications
You must be signed in to change notification settings - Fork 58
ADD: removeMany case to _run function (internal) #7
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
Open
meengit
wants to merge
1
commit into
db-migrate:master
Choose a base branch
from
meengit:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -325,12 +325,11 @@ var MongodbDriver = Base.extend({ | |
else | ||
db.collection(collection).insertOne(options, {}, callbackFunction); | ||
break; | ||
case 'removeMany': | ||
db.collection(collection).deleteMany(options, callbackFunction); | ||
break; | ||
case 'remove': | ||
// options is the records to insert in this case | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing and altetering functionality outside a deprication cycle is not allowed, therefore adding a new method is fine, but removing this is not allowed |
||
if(util.isArray(options)) | ||
db.collection(collection).deleteMany(options, callbackFunction); | ||
else | ||
db.collection(collection).deleteOne(options, callbackFunction); | ||
db.collection(collection).deleteOne(options, callbackFunction); | ||
break; | ||
case 'collections': | ||
db.collections(callbackFunction); | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 probably be called
deleteMany
. Makes more sense, no?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.
Thanks for helping on the review :)
The other case is named remove, @marc0der probably oriented after that, so wouldn't be an issue. This is an internal method anyway, but I agree that the mongodb needs some love. If you would be willing to give it a shot you're as always more than welcome to so :)
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.
Yes, exactly as @wzrdtales says.
@wzrdtales would you hit that merge button please :-) It's been open since 1 September 2017.
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.
@marc0der Nope. There are no tests, if you want you can create a fork from his tree and add them and put this into a new PR and I will he happy to merge :)
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.
Also the altering of remove needs to be removed as this would be a breaking change.
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.
I know that is why I told to fork :)
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.
Don't worry about it, I've already opted for using a different migration framework.
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.
That is totally ok, I always appreciate people contributing to open source though :)
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.
As do I, in fact I was using your open source project in my own open source project ;-)
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.
Wasn't a critique though, I'm totally fine if anyone uses something else instead. Open Source means freedom, but shouldn't be considered to be "for free". Everyone and every comany should contribute back, unfortunately many do not. I ment actually this specific PR of @meengit here which I do highly appreciate and I just mentioned it b/c this has been open for a really long time, which I'm sorry for. And the same reason why this has been lying around is which makes me appreciate people that help. I do not have unlimited time, so everyone that contributes is a tremendous help. And that is quite true for every open source project that is not backed by money or a company :)