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

Conversation

Projects
None yet
2 participants
@mihails-strasuns
Contributor

mihails-strasuns commented Jan 27, 2013

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 some commits Jan 26, 2013

Dicebot
Main refactoring:
    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
Dicebot
Main refactoring
    1) opIndex for MongoDatabase
@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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

Contributor

mihails-strasuns commented Jan 27, 2013

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

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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

Contributor

mihails-strasuns commented Jan 27, 2013

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

@s-ludwig

View changes

Show outdated Hide outdated source/vibe/db/mongo/collection.d
@s-ludwig

View changes

Show outdated Hide outdated source/vibe/db/mongo/connection.d
@s-ludwig

View changes

Show outdated Hide outdated source/vibe/db/mongo/connection.d
@s-ludwig

View changes

Show outdated Hide outdated source/vibe/db/mongo/db.d
@s-ludwig

View changes

Show outdated Hide outdated source/vibe/db/mongo/db.d
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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).
Member

s-ludwig commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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.
Contributor

mihails-strasuns commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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").

Member

s-ludwig commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

"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.

Contributor

mihails-strasuns commented Jan 27, 2013

"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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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)...

Member

s-ludwig commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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

Member

s-ludwig commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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.

Contributor

mihails-strasuns commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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.

Contributor

mihails-strasuns commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

@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 :)

Contributor

mihails-strasuns commented Jan 27, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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.

Member

s-ludwig commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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.

Contributor

mihails-strasuns commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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 ;)

Member

s-ludwig commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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.

Contributor

mihails-strasuns commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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.

Member

s-ludwig commented Jan 27, 2013

[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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

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?

Contributor

mihails-strasuns commented Jan 27, 2013

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

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Jan 27, 2013

Contributor

Ok I thing I got this right finally ;)

Contributor

mihails-strasuns commented Jan 27, 2013

Ok I thing I got this right finally ;)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 27, 2013

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.

Member

s-ludwig commented Jan 27, 2013

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