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

Mongo driver refactoring #171

Merged
merged 11 commits into from Jan 27, 2013
Merged

Mongo driver refactoring #171

merged 11 commits into from Jan 27, 2013

Conversation

mihails-strasuns
Copy link
Contributor

Wow, it was much faster and much easier than I have expected. Current pull request status: basic changes, "works for me". Probably some enhancement commit will be added if review will take some time, otherwise those will go as separate ones.

Changes copied from git log:

1) vibe.db.mongo.db -> vibe.db.mongo.client
       MongoDB -> MongoClient
       MongoClient is cleaned up from all database specific features
       MongoClient.opIndex removed
       MongoClient.getDB added
    2) Created new module vibe.db.mongo.db
       Created MongoDatabase class
       All database service functions moved to MongoDatabase
    3) Most vibe.db.mongo.connection stuff is marked as /// private
       All vibe.db.mongo.connection symbols intended for user code usage are documented
    4) getLastError() root implementation is now in MongoConnection
       getLastError() implementation fixed
       getLastError() now return immutable POD struct: MongoErrorDescripion
       checkLastError() now uses getLastError()
    5) MongoHost class -> struct
    6) MongoClient now only uses MongoClientSettings in constructor
    7) MongoCollection stores MongoDatabase instead on plain name string
    8) MongoDatabase basic implementation with old service methods: getLastError, 
    9) MongoDatabase opIndex
getLog, fsync
    9) MongoDatabase.runCommand is package instead of public for better encapsulation

Enhancments:
    1) Added MongoDriverException and MongoDBException
       All mongo driver code now throws one of those
       MongoDBException encapsulated MongoErrorDescripion
    2) MongoCollection.remove() overload added

Dicebot added 4 commits January 27, 2013 01:44
    1) vibe.db.mongo.db -> vibe.db.mongo.client
       MongoDB -> MongoClient
       MongoClient is cleaned up from all database specific features
       MongoClient.opIndex removed
       MongoClient.getDB added
    2) Created new module vibe.db.mongo.db
       Created MongoDatabase class
       All database service functions moved to MongoDatabase
    3) Most vibe.db.mongo.connection stuff is marked as /// private
       All vibe.db.mongo.connection symbols intended for user code usage are documented
    4) getLastError() root implementation is now in MongoConnection
       getLastError() implementation fixed
       getLastError() now return immutable POD struct: MongoErrorDescripion
       checkLastError() now uses getLastError()
    5) MongoHost class -> struct
    6) MongoClient now only uses MongoClientSettings in constructor
    7) MongoCollection stores MongoDatabase instead on plain name string
    8) MongoDatabase basic implementation with old service methods: getLastError, getLog, fsync
    9) MongoDatabase.runCommand is package instead of public for better encapsulation

Enhancments:
    1) Added MongoDriverException and MongoDBException
       All mongo driver code now throws one of those
       MongoDBException encapsulated MongoErrorDescripion
    2) MongoCollection.remove() overload added
    1) opIndex for MongoDatabase
@mihails-strasuns
Copy link
Contributor Author

Also main site docs need to be updated. Will add related pull request later.

@mihails-strasuns
Copy link
Contributor Author

Also please note answer in newsgroup regarding possible CI and functional / sanity tests.

doc["name"] = indexname.data;
if( flags & IndexFlags.Unique ) doc["unique"] = true;
if( flags & IndexFlags.DropDuplicates ) doc["dropDups"] = true;
if( flags & IndexFlags.Background ) doc["background"] = true;
if( flags & IndexFlags.Sparse ) doc["sparse"] = true;
m_db[databaseName ~ ".system.indexes"].insert(doc);
m_client.getCollection(database.name ~ ".system.indexes").insert(doc);
Copy link
Member

Choose a reason for hiding this comment

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

m_db["system.indexes"].insert(doc)?

@s-ludwig
Copy link
Member

Great, that was fast ;)
Looking good so far, two things I've noticed:

  • I'm not so sure about the /// private comments there. Usually I try to keep even special classes available for use and consider a comment sufficient stating that usually the high-level functionality should be preferred. I have to admit though that I'm also filtering out vibe.core.drivers in the online docs... However, I don't remember why I did that.
  • MongoDatabase looks prettier than MongoDB and avoids confusion with the trademark name, which is good. The module name and possibly getDB() should be renamed accordingly (whenever it makes sense, I try to keep class name == module name for consistency).

@mihails-strasuns
Copy link
Contributor Author

Will address comments soon today.

  1. Well, they are not private -> available for use. My overall position is to make generated docs as friendly as possible to newbies and leave normal (not-ddoc) comments in code to power users (they will explore sources anyway). As you may notice there are some symbols in connection.d that do propagate to user code and those are not hidden from docs. For others I am just trying to help newbies to not shoot in their foot.
  2. MongoDB -> MongoDatabase was done exactly to avoid confusion with trademark, but I left usual plan DB == database abbreviation in some other cases because it did not feel that ambiguous. Not strong preferences here so will renamed as per your request.

@s-ludwig
Copy link
Member

Maybe regarding 1., it would make sense to introduce another DDOC thing that marks an entity as internal? Then it could be filtered out or displayed with a red "internal" badge.

Generally, my approach is to document everything that is interesting for development - be it pure application development or developing the library itself. Filtering can then be done when generating the docs (i.e. another alternative in this case would be to filter out the whole vibe.mongo.connection module.

For the DB<->Database thing, at least the module should be named accordingly. Regarding getDB I'm personally not completely sure yet, but keeping it consistent seems like it has the chance to make the API more consistent (read "intuitive").

@mihails-strasuns
Copy link
Contributor Author

"to introduce another DDOC thing that marks an entity as internal?" - awesome, that will solve this problem nicely. Is it a big effort to add one?

I must admit initially I have marked all vibe.db.mongo.connection as package :) But
a) It has few low-level symbols like exception and MongoErrorDescription that are supposed to be used and documented. I did not feel like creating another module only for those is justified.
b) There is no point in prohibiting to mess with connection for one who knows what he is doing.

internal or advanced ddoc mark will help a lot.

@s-ludwig
Copy link
Member

It should be quite quick tp add. One question would be which syntax to use. /// [internal] This is the description looks nice, but would appear in standard DDOC output as-is (but maybe that's actually not such a bad thing).

An alternative using macros: /// $(DDOX_TAG internal) This is the description - ugly, but standard DDOC could format or exclude it however it wants.

I'm leaning towards the first variant in favor of readability (a credo of DDOC that is taken ad adsurdum in many parts of the Phobos docs already)...

@s-ludwig
Copy link
Member

Hm a thrid option would be another special section:

/** Desctiption.

    Tags: internal
*/

Has the drawback that it is a bit more tedious to implement

@mihails-strasuns
Copy link
Contributor Author

I will use any solution implemented :) Final documentation quality is what more important to me and I doubt someone will use DDOC instead of DDOX for vibed.org doc generation.

@mihails-strasuns
Copy link
Contributor Author

One argument in favor of 3d, more difficult solution is that it allows to do a normal good documentation for everything and easily control final target output via tag combination. I.e. make special developer documentation with private and internal symbols included and still with all good info.

@mihails-strasuns
Copy link
Contributor Author

@s-ludwig
Btw, is there vibe.d style guide? My vim is configured to follow Phobos style guidelines and now all of mongo modules got reformatted from tabs to space :)

@s-ludwig
Copy link
Member

Unfortunately not yet really. There are some beginnings in an internal wiki though. I will put them on the github wiki as soon as they are a bit more finalized. But yeah, it's tabs for indentation. That was somthing I forgot to ask, since github converts anything to spaces I couldn't tell.

@mihails-strasuns
Copy link
Contributor Author

OK, all but internal related stuff done. Sanity project is added with simple asserts, I was to lazy too write anything special until CI testing suite is introduced.

Awaiting for your resolution with new DDOX keyword.

@s-ludwig
Copy link
Member

Okay, lets just go with the /// [tag1, tag2, ...] Rest of the comment syntax. Using a macro is ugly in the input text and the DDOC result would depend on its position inside of the doc comment. And the section solution is also kind of ugly, as the section would have to be filtered out everywhere, the DDOC result would also not be quite as nice as for the other solutions (having a section with only one word in it).

Regarding the tag name itself internal seems a tad bit too strong, but seems OK. advanced seems to imply that using the tagged stuff is something like an intended alternative with more control, so im leaning towards the former. (And BTW, I hate these naming issues ;)

@mihails-strasuns
Copy link
Contributor Author

Changes some stuff to use new tag. Leaving up to you to chose which symbols should be [private] and which [internal], though, it is hard to decide when you are not a class author.

@s-ludwig
Copy link
Member

[private] (or /// private) is just a workaround for DMD issues, namely for templates, where the JSON output does not contain a protection attribute. Hopefully that will not be necessary anymore with the ongoing fixes by Walter in that area.

@mihails-strasuns
Copy link
Contributor Author

Oh, I am so bad then :) Was sure it was added to hide public symbols for ddoc generation.
So all connection module excluding exception and eror description can be marked as internal?

@mihails-strasuns
Copy link
Contributor Author

Ok I thing I got this right finally ;)

@s-ludwig
Copy link
Member

Ok, looks ready to merge. I'll do that and then go fix all of my projects. Any possible fixes can still be made later.

s-ludwig added a commit that referenced this pull request Jan 27, 2013
@s-ludwig s-ludwig merged commit 18475f1 into vibe-d:master Jan 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants