Skip to content
This repository has been archived by the owner on Aug 16, 2018. It is now read-only.

move database to class extension #5

Merged
merged 3 commits into from
Oct 17, 2015
Merged

move database to class extension #5

merged 3 commits into from
Oct 17, 2015

Conversation

bsouthga
Copy link
Contributor

Add TemporalDatabase as a decendent of promised-mongo's Database.

(also adds aggregate api fix to emulate promised-mongo's approach: https://github.com/gordonmleigh/promised-mongo/blob/master/lib/Collection.js#L40-L43

@yangchristian
Copy link
Member

Great! This looks like a nice, low risk refactor. Merging now 👍

aggregateByDate(pipeline={}, options, date) {
return this.aggregateRaw({ ...pipeline, $match: addDate(pipeline.$match, date) }, options);
aggregate(...pipeline) {
return this._aggregateWrapper(addCurrent, null, pipeline);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused by the spread operators. Shouldn't the first argument to aggregate be an array already?

While it's true that promised-mongo passes an object in the example you linked (and in their aggregate test, I think this is actually a bug and should be passing an array (which is the Mongo API):
gordonmleigh/promised-mongo#27

Our app code uses that array format too.

@bsouthga
Copy link
Contributor Author

updated to require array pipeline

yangchristian added a commit that referenced this pull request Oct 17, 2015
move database to class extension
@yangchristian yangchristian merged commit f65646f into master Oct 17, 2015
@bsouthga bsouthga deleted the oo-database branch October 17, 2015 14:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants