New MongoDB Index Management API #2433
Conversation
There was a problem hiding this comment.
This is great work! I wanted to tackle this for a long time, but never got around to it, which instead forced be to stop upgrading MongoDB on the services that currently use it.
In general I think everything looks fine, and apart from the question of where to place the API facade, the only thing that would be nice to have are some minimal documented unittest examples for the different createIndex variants (e.g. just an embedded test() { ... } function that features only the call to createIndex, nothing really functional).
mongodb/vibe/db/mongo/collection.d
Outdated
| enforce(reply["ok"].get!double == 1, "getIndexes command failed: "~reply["errmsg"].opt!string); | ||
| return MongoCursor!R(m_client, reply["cursor"]["ns"].get!string, reply["cursor"]["id"].get!long, reply["cursor"]["firstBatch"].get!(Bson[])); | ||
| } | ||
| mixin MongoCollectionIndexStandardAPIImpl; |
There was a problem hiding this comment.
I don't think this will work with the documentation and I'd stylistically prefer to have concrete methods here (which could simply be proxies to the implementation in the internal module). This would create a slightly unattractive cyclic dependency, unless the accompanying type declarations are stored in a separate module, but I wouldn't mind it too much, as that can still be changed at any point.
There was a problem hiding this comment.
Thanks for the review! I think using mixin templates here is really the D way to go. When I plan to implement the other APIs (see #2200) it would massively fill this collection.d file
The inline documentation & type definitions alone will probably make up over half of the file then. Having them in dedicated modules improves code maintainability imo.
The MongoDB driver spec also has separate documents for all the different features a driver may implement, having corresponding D modules for this is another plus.
Having separate implementation files is okay, but for example you would either need to omit the documentation for the methods or duplicate the documentation in the documentation file. I think an attractive pro argument of using the mixin templates is that you have both code and documentation in one place without duplication, which makes development a little more convenient. This is kind of D's equivalent to C#'s partial classes.
So I think we should be using D's great features here and if there is a problem with tooling/documentation rather fix that to support D better.
There was a problem hiding this comment.
There is also the alternative of using UFCS and freestanding functions in a separate module. As far as I can see, that would solve both issues.
There was a problem hiding this comment.
hm I'm not a big fan of UFCS for this kind of functionality because it often breaks discoverability of the functions (auto complete, documentation)
I tested now how the documentation output looks in ddox: for the mixin template it looks just like a class with all the member functions, which I think is pretty good and works just fine.
How about we just link to all mixin'd templates at the top of the class definition?
See also: dlang/ddox#152
There was a problem hiding this comment.
I would usually expect a mixin to have worse issues with auto-completion than a UFCS function, but I have no idea how this is distributed across real-world implementations. It's good to see that the mixed-in members appear correctly in the documentation, but I still don't really like this stylistically and would have to think more about it before coming to a final opinion.
So then let's just keep the methods as normal members for now, so that the non-controversial parts can be merged.
There was a problem hiding this comment.
moved all the members into the class now so it's uncontroversial
|
Okay, looks good to me now. I'll merge this and then tag 0.9.0-rc.1. Can you just perform a final rebase to get rid of the merge commit? |
Use with new enforceWireVersionConstraints function to nullify values that don't fall in the expected server version.
This copies some logic from the opAssign methods, but given how useful init objects for CTFE may be, this is a good trade-off
Move all index implementation from mixin template into collection class
|
ok rebased, want me to squash this into a few commits too? |
|
Thanks! The history looks clean enough IMO, considering that shuffling around commits might get a little hairy due to the code moves. |
Properly implements & tests the index API according to next gen driver spec and backwards compatible dropDups from MongoDB <=2.6
Only testing this on one MongoDB version though, might make sense to try different supported mongodb version on travis