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

Use ActiveSupport Concern #289

Merged
merged 3 commits into from
Oct 29, 2013
Merged

Use ActiveSupport Concern #289

merged 3 commits into from
Oct 29, 2013

Conversation

chulkilee
Copy link
Contributor

It makes easy to use version class with different AR connection

@seanlinsley
Copy link
Member

Can you give an example where you want to use this?

@chulkilee
Copy link
Contributor Author

# with concerns - able to inherit custom Base class and share connections
module Foo
  class Base < ActiveRecord::Base
  end

  class Version < Base
    include PaperTrail::VersionConcern
  end

  class Document < Base
    has_paper_trail class_name: 'Foo::Version'
  end
end

module Bar
  class Base < ActiveRecord::Base
  end

  class Version < Base
    include PaperTrail::VersionConcern
  end

  class Document < Base
    has_paper_trail class_name: 'Bar::Version'
  end
end

Foo::Base.establish_connection({})
Bar::Base.establish_connection({})

Without concerns, version classes need to have it's own connection because PaperTrail::Version will use ActiveRecord::Base's connection. In addition it's difficult to add common behaviors or override ActiveRecord::Base method on Foo::Version using Foo::Base, because it inherits PaperTrail::Version (and ActiveRecord::Base), not Foo:Base.

# without concerns - need to inherit PaperTrail::Version
module Foo
  class Version < PaperTrail::Version
  end
end
module Bar
  class Version < PaperTrail::Version
  end
end

[Foo::Base, Foo::Version].each { |klass| klass.establish_connection({database: 'foo'}) }
[Bar::Base, Bar::Version].each { |klass| klass.establish_connection({database: 'bar'}) }

@batter
Copy link
Collaborator

batter commented Oct 22, 2013

So essentially this just makes it easy to use the gem with an application that makes multiple connections to databases. I think this makes sense then since it leaves the original functionality in tact but adds some flexibility. Any chance you could add a test or spec demonstrating the functionality if it's not too much trouble? If nothing else, it makes the use case easy to see for others going forward.

@ghost ghost assigned batter Oct 22, 2013
@jherdman
Copy link

FYI it's idiomatic usage of the AS::Concern module to define your class-level methods in a module called ClassMethods, and not in the included block. Example:

module VersionConcern
  included do
    # macros in here
  end

  module ClassMethods
    def some_class_level_method
    end
  end

  def some_instance_level_method
  end
end

@chulkilee
Copy link
Contributor Author

PaperTrail::Version already uses the concern and all existing tests are passed - what kind of tests can we add? any suggestion?

@jherdman I'll make that change - thanks!

It makes easy to use version class with different AR connection
@batter
Copy link
Collaborator

batter commented Oct 22, 2013

@chulkilee - Probably just something similar to your example above, where a custom version class includes the PaperTrail::VersionConcern.

Also, it looks like the portion with the class methods needs to be modified a bit. The object_col_is_json? and object_changes_col_is_json? methods are now class methods. Does that fix all the failing tests?

@chulkilee
Copy link
Contributor Author

@fullbridge-batkins fixed and added tests

@batter
Copy link
Collaborator

batter commented Oct 28, 2013

Yes I saw that, thank you, looks good. Was busy last week but going to merge this soon.

@batter batter merged commit 00bf38f into paper-trail-gem:master Oct 29, 2013
@batter
Copy link
Collaborator

batter commented Oct 29, 2013

Thanks a lot for this pull, very cool feature. I'm wondering if there is some way to document the usage of this on the README a bit better but perhaps the specs you provided make it obvious. By the way, I refactored the specs a little bit in 975e1bb so it is organized a bit better.

@chulkilee
Copy link
Contributor Author

thanks!

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.

4 participants