search

vapor-community / mongo-driver

MongoDB driver for Fluent
28 stars 26 forks source link

Update to MongoKitten 3 and support latest fluent count() feature #29

Closed fedefrappi closed 7 years ago

fedefrappi commented 7 years ago

I built this on top of @Joannis #26 by updating to the official mongokitten 3 release and fixing some other issues (e.g. missing bson to bool conversion). I also added support for the count() feature recently added to fluent. Finally the underlying mongokitten driver is now public so that users are able to use it directly for implementing features currently not available on fluent (e.g. geoqueries and indexes).

Joannis commented 7 years ago

I didn't read the code, but I completely agree with the changes you've noted down in the commits and the accessibility of the underlying driver.

tanner0101 commented 7 years ago

@fedefrappi this looks great.

@Joannis is MongoKitten 3 officially released?

Joannis commented 7 years ago

@tannernelson it sure is!

Joannis commented 7 years ago

I'd prefer the next tag (if it's a major one) to also cover ObjectId properly. Currently people that make references between databases have the problem that ObjectId is converted to a Node.string but the reverse isn't possible. I'm thinking the ExtendedJSON approach might be fit for this task.

tanner0101 commented 7 years ago

Has this been tested on macOS and Linux? Unfortunately this repo doesn't have travis setup yet.. I'd definitely like to verify everything checks out before we tag it.

fedefrappi commented 7 years ago

The compiler started segfaulting with one of the latest releases but it is fixed now. All tests pass on macOS (some are new with this pull requests). Do you have a quick way for running the tests on Linux as well?

Joannis commented 7 years ago

@fedefrappi we had a bug in MongoKitten recently causing the compiler not to be able type check an initializer without crashing. That's solved now.

fedefrappi commented 7 years ago

@Joannis thanks! I just tried updating all packages and all tests are still passing fine :)

tanner0101 commented 7 years ago

@fedefrappi I invited you to the Fluent team. Once accepted, could you move this PR on a branch in this repo?

This will help me test so we can get this merged. Thank you. :)

fedefrappi commented 7 years ago

@tannernelson sure but I don't think I received the invitation? Thanks!

vzsg commented 7 years ago

I had that too, be sure to check https://github.com/vapor, the invitation should pop up in the header section.

fedefrappi commented 7 years ago

@vzsg thanks that worked! I'll move the pull request to another branch soon :)

fedefrappi commented 7 years ago

@tannernelson new branch created and base branch changed, will you take it from here?