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

replaced the deprecated findAndModify method from native js driver to… #448

Merged
merged 1 commit into from Jun 22, 2017

Conversation

@lushc
Copy link
Member

lushc commented Jun 22, 2017

… findOneAndUpdate

Closes #268

@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 22, 2017

Travis failed for Node.js v0.11 tests — how about we just ditch v0.10 and v0.11 going forward?

Edit: #449

@lushc

This comment has been minimized.

Copy link
Member Author

lushc commented Jun 22, 2017

I'm not entirely sure why it failed and if it's down to the Node version, the native driver still supports v0.10 and v0.12, so I'm assuming it was just an intermittent connection issue (they mention this in the docs) which backs up the actual build error.

@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 22, 2017

@lushc if you wanna debug/test that, you can close and re-open to re-run Travis.

@lushc

This comment has been minimized.

Copy link
Member Author

lushc commented Jun 22, 2017

Ah, that'd be much quicker than testing it in a TravisCI docker container like I just did...

@lushc lushc closed this Jun 22, 2017
@lushc lushc reopened this Jun 22, 2017
@lushc

This comment has been minimized.

Copy link
Member Author

lushc commented Jun 22, 2017

@simison all passing this time, rather than removing these versions do you think we should mimic the native driver and test against 0.12 instead of 0.11? Although they have been end-of-life since late 2016, so maybe removing them is better, not sure.

@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 22, 2017

0.12 might just pass or bounce again randomly, I'm not sure if it's really worth effort. They're end-of-life for exactly this reason that people wouldn't need to keep supporting them as it's time consuming. ;-)

@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 22, 2017

...I'd merge this without rebasing (to remove 0.11) and then merge #449 later if no-one has anything against it.

@lushc

This comment has been minimized.

Copy link
Member Author

lushc commented Jun 22, 2017

Oh I didn't remove 0.11 by the way, so #449 can be merged whenever. The build failure was just an intermittent issue where Mongo wasn't ready by the time the tests started running, which is a bit odd as the build already waits 15 seconds for it. This has happened to me before on other projects so I'm probably cursed :P

@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 22, 2017

@lushc apparently the same issue with Node.js v0.11 occurred here: #331 (comment)

@simison simison merged commit b818de4 into agenda:master Jun 22, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@simison

This comment has been minimized.

Copy link
Member

simison commented Jun 22, 2017

Thanks a bunch btw for this! Happy to get some actual code merged in finally. :-)

@lushc lushc deleted the Codevate:feature/deprecated-findandmodify branch Jun 22, 2017
@lushc

This comment has been minimized.

Copy link
Member Author

lushc commented Jun 22, 2017

No problem, you've really managed to get the ball rolling on triaging issues/PRs and getting an active core of maintainers together, so thank you 👍 🍺 a remarkable difference compared to early last week!

{$set: {lockedAt: now}}, // Doc
{'new': true}, // options
{returnOriginal: false, 'priority': -1}, // options & sort

This comment has been minimized.

Copy link
@lushc

lushc Jun 22, 2017

Author Member

I've just noticed that according to the findOneAndUpdate docs this is incorrect. It should be:

{returnOriginal: false, sort: {priority: -1}}

There doesn't seem to be any test coverage for this either. I'll put in another PR to fix.

This comment has been minimized.

Copy link
@simison

simison Jul 5, 2017

Member

Follow ups #451

@simison simison added this to the 0.10.0 milestone Jul 5, 2017
simison added a commit that referenced this pull request Jun 11, 2018
Deprecated `findAndModify()` has already been updated to `findOneAndUpdate()` in #448
simison added a commit that referenced this pull request Jun 11, 2018
Deprecated `findAndModify()` has already been updated to `findOneAndUpdate()` in #448
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.