ADD: removeMany case to _run function (internal) #7
Conversation
# FIX
If you had run till now with something like:
`db._run('remove', 'collectionName', { myKey: 'string' });`
you couldn't remove many documents.
The code to delete many implied to use it as:
`db._run('remove', 'collectionName', [{ myKey: 'string' }]);` (check for `util.isArray(options)`)
but that would throw an error [1].
To keep current code running and not to break stuff I introduced the `removeMany`:
`db._run('removeMany', 'collectionName', { myKey: 'string' })`
1: [db.collection.deleteMany()](https://docs.mongodb.com/manual/reference/method/db.collection.deleteMany/)
meengit
changed the title
|
I will review this a bit later, short on time currently. |
|
Could this be reviewed and possibly merged please? This would help me a lot in solving a problem I currently have. |
| @@ -325,12 +325,11 @@ var MongodbDriver = Base.extend({ | |||
| else | |||
| db.collection(collection).insertOne(options, {}, callbackFunction); | |||
| break; | |||
| case 'removeMany': | |||
There was a problem hiding this comment.
Should probably be called deleteMany. Makes more sense, no?
There was a problem hiding this comment.
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.
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.
@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.
Also the altering of remove needs to be removed as this would be a breaking change.
There was a problem hiding this comment.
I know that is why I told to fork :)
There was a problem hiding this comment.
Don't worry about it, I've already opted for using a different migration framework.
There was a problem hiding this comment.
That is totally ok, I always appreciate people contributing to open source though :)
There was a problem hiding this comment.
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.
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 :)
| case 'remove': | ||
| // options is the records to insert in this case |
There was a problem hiding this comment.
Removing and altetering functionality outside a deprication cycle is not allowed, therefore adding a new method is fine, but removing this is not allowed
|
Thank you very much for reviewing and commenting. The pull request is open since Sep 1, 2016, or Release v1.1.4 (Feb 5, 2016). If I saw it correctly, tests were not present at this version. In the meantime, we do not need the db-migrate anymore. However, I am interested writing the tests. I am not familiar with the project anymore. I have to familiarize myself again. Unfortunately, I am not able to do this before Monday, Feb 19, 2018. Please let me know if I should write. |
|
@meengit Yes this has been open for too long that is very true. If you want to write the tests for this that would be fantastic and I can just appreciate your spirit 🦄 even though it has been open for way too long. |
I couldn't delete multiple documents. I know
_runis private, but still thought it would be helpful to suggest to fix it:If you had run till now with something like:
db._run('remove', 'collectionName', { myKey: 'string' });you couldn't remove many documents.
The code to delete many implied to use it as:
db._run('remove', 'collectionName', [{ myKey: 'string' }]);(check forutil.isArray(options))but that would throw an error [1].
To keep current code running and not to break stuff I introduced the
removeMany:db._run('removeMany', 'collectionName', { myKey: 'string' })1: db.collection.deleteMany()
What do you think? Or did you have an other idea when you checked for
util.isArray(options)?