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

Make PaperTrail a Rails engine #347

Closed
wants to merge 1 commit into from
Closed

Make PaperTrail a Rails engine #347

wants to merge 1 commit into from

Conversation

yuki24
Copy link

@yuki24 yuki24 commented Mar 20, 2014

I've read #216 and you might think it is unnecessary, but it does provide some benefits.

Gems like kamianri assume that every AR model is lazily loaded on Rails in development or auto-loaded in production. With this lazy loading mechanism, we can implement some useful features. For example, kaminari allows the user to change the page method name(by default it is .page) to whatever the user wants to call. However, since the PaperTrail::Version class is manually required by the gem, it can't take the advantage of this mechanism. As a result, kaminari fails to define the page method with the right name(see: https://github.com/amatsuda/kaminari/pull/262).

It sounds like it is just a kaminari/paper_trail-only issue, but it also could happen to any other gems. To solve that issue, we can make PaperTraila a Rails engine so Rails will automatically load the PaperTrail::Version class. Could you think about merging this request?

@batter
Copy link
Collaborator

batter commented Mar 20, 2014

Thanks for the Pull Request. Is there any way you could write some automated tests for this demonstrating how the behavior changes as a result of this? Also, the gem is purposely decoupled from Rails (or at least doesn't require rails by default) so that it can be used outside of Rails if the user chooses to do so, so this class would need to go in lib/frameworks/rails/engine.rb or something to that effect I would think. Cheers!

@yuki24
Copy link
Author

yuki24 commented Mar 20, 2014

We can still use PaperTrail::Version if we make the change below in paper_trail.gemspec:

-  s.require_paths = ['lib']
+  s.require_paths = ['lib', 'app/models']

But this will require us to do require 'paper_trail/version' before we actually use it, we can let PaperTrail do it if Rails is not defined, though.

And yes, PeperTrail::Engine could be moved to lib/frameworks/rails/engine.rb.

Speaking of automated tests, we can definitely do it. However, I can't come up with a nice test for PaperTrail::Version. I can add the ability to configure and dynamically change current_user in lib/frameworks/rails.rb to something e.g. current_person by applying this pattern, but I think this is outside of the scope of this pull request.

@batter batter added this to the 3.1.0 milestone Apr 16, 2014
@batter
Copy link
Collaborator

batter commented May 23, 2014

@yuki24 - Sorry for the delayed response. Now that I have time to look at this a little more closely, I think I agree that it makes sense to make the gem an engine when used inside of Rails, however, your PR failed to pass on Travis, and I'm not sure it's a complete implementation. Is there more that needs to be done here to make this work properly? I'm going to try to do some reading on Rails Engine implementations, but I want to make sure this case will cover issues like amatsuda/kaminari#262.

@batter
Copy link
Collaborator

batter commented May 23, 2014

Hey, so I just took a crack at cleaning up the groundwork you had done for this. Does this look like it will handle the type of issue from that Kaminari issue report? Here is the commit on a branch

@batter batter modified the milestones: 3.0.3, 3.1.0 Jun 5, 2014
@batter batter closed this in f3f5925 Jun 5, 2014
batter referenced this pull request Aug 6, 2014
…w to use a custom initializer and prevent breakage of the Rails::Engine functionality [ci skip]
@batter batter modified the milestones: 3.1.0, 3.0.3 Oct 9, 2014
@yuki24 yuki24 deleted the make-paper_trail-rails-engine branch March 24, 2017 01:15
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.

2 participants