-
Notifications
You must be signed in to change notification settings - Fork 336
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
MongoMapper support #101
MongoMapper support #101
Conversation
|
||
# Module extending classes that serve as owners | ||
module ClassMethods | ||
# Adds ActiveRecord associations to model to simplify fetching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you adjust those comments to mention MongoMapper and not AR?
Fine job! I will review this with other changes I have for Rails 4.0 and adjust after merging. I want to release this with bigger changes for final Rails version. |
Hi, Does MongoMapper support |
That's great, it means we don't need to write different methods for all ORMs (so far, at least). Also, could you change the README so that it shouts MongoMapper too? 😄 |
There you go, the only place where I didn't add MongoMapper was on the upgrade section of the README, since if you have a version older than 0.4 you wouldn't have support for MongoMapper anyway. In any case, if you think it should go there, let me know. |
Great, thank you. We will be pulling it upstream shortly :-)
|
Have you tested hooks functionality? I'm yet to test this myself, but I wanted to ask, since we use after_create and similar AR style hooks. |
I understand that they implemented all the same callbacks using ActiveModel::Callbacks. I didn't have the time yet to test this PR on a project using MongoMapper so I can't be entirely sure if anything outside the scope of the specs is working. |
@farnoy the tests are passing so I don't see why this wouldn't work. |
Thank you for this big input! I will start work on merging my Rails 4 branch and prepare for a bigger release. 🍺 |
include ::MongoMapper::Document | ||
include Renderable | ||
|
||
class SymbolHash < Hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make another PR with some documentation for this & other entries defined by #101? Comments would significantly increase maintainability for future contributors.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add them in the next few days when I have some free time. I've been taking a look at the PR; besides this change, the others seem to be either the equivalent of Mongoid or quite straightforward (like the explicit order at https://github.com/pokonski/public_activity/pull/101/files#L7R6), is there any other place in particular that you think deserves a comment?
Changes Unknown when pulling c938cc1 on julioolvr:mongomapper-support into * on pokonski:master*. |
Refs #81
I used the latest version of the gem available at Rubygems, 0.12.0. Maybe it works with older versions, I didn't test it.
Also, I'm in no ways an expert with Mongo, Mongoid or MongoMapper. Mongoid and MongoMapper are quite similar so I pretty much copy-pasted, changed the syntax and then checked the remaining differences that the test brought up.
I didn't update the documentation, figured that could wait until the code is reviewed. For executing the tests the only thing needed is setting the environment variable
PA_ORM
tomongo_mapper
, and to use it in an app just doorm :mongo_mapper
in the config block. I haven't tested it on a real app yet but I'll probably do it sometime this weekend.