Skip to content
This repository has been archived by the owner on Nov 12, 2019. It is now read-only.

Do not load if ActiveRecord not present #3

Merged
merged 2 commits into from
Nov 20, 2013

Conversation

johnnyshields
Copy link
Contributor

Polyamorous is a gem dependency for a number of libraries. Currently, loading Polyamorous forces loading ActiveRecord, which I don't want to do.

So instead of forcing to require active_record in Polyamorous, I've changed it so that if AR is not defined, Polyamorous doesn't load (i.e. does nothing).

@ernie
Copy link
Contributor

ernie commented Sep 5, 2013

Polyamorous only makes sense in cases of ActiveRecord being used. Its sole purpose is to monkey-patch ActiveRecord to support polymorphic belongs_to joins.

@ernie ernie closed this Sep 5, 2013
@johnnyshields
Copy link
Contributor Author

@ernie I agree with you that Polyamorous only makes sense if AR is being used.

However, the problem is that the gem forces AR to load when Polyamorous loads. Anytime I have another gem which calls Polyamorous as dependency, AR loads whether I want it to or not. (ActiveAdmin for example does this)

I'd like to change the behavior here so that Polyamorous is essentially in silent mode if AR is not already loaded.

@johnnyshields
Copy link
Contributor Author

@ernie an alternative would be to make ransack / metawhere gems only load polyamorous if AR is present. However, I don't think that's possible given how bundler works.

@johnnyshields
Copy link
Contributor Author

@daxter ActiveAdmin depends on Ransack gem, which in turn depends on Polyamorous and MetaSearch

Recently we've made AA to be ORM agnostic. However, Polyamorous and MetaSearch both force ActiveRecord to load, even when the user does not want it.

As far as I know there's no way to remove Ransack from ActiveAdmin on a per-ORM basis without removing it from the AA gem bundle and making user require it externally (not good). In this PR I'm suggesting that Polyamorous should no longer require AR, and instead should go "silent" if AR is not already required in the Gemfile--this is the practice in many other gems. Can you suggest if there's any alternative?

MetaSearch PR is here: activerecord-hackery/meta_search#118

@johnnyshields
Copy link
Contributor Author

@ernie can you please reconsider this? I have over 150 gems in my Rails project, and your two (polyamorous and metawhere) are the only two which force ActiveRecord to load. Even ActiveRecord doesn't load itself until you call the Railtie in your app initializer.

@seanlinsley
Copy link

@johnnyshields neither meta_where nor meta_search has been maintained in the past two years. If you're still using them in projects, you're going to have to patch your own versions.

As far as polyamorous, are you using it (via Ransack) for anything other than Active Admin? Just trying to gauge the scope.

And if you weren't aware, I renamed my account from Daxter to seanlinsley

@johnnyshields
Copy link
Contributor Author

@seanlinsley looking at http://rubygems.org/gems/activeadmin, ActiveAdmin still depends on meta_search ~> 1.0, which in turn depends on polyamorous ~> 0.5.0

I am not using these for other purposes besides ActiveAdmin.

It's trivially easy to wrap the meta_search and polyamorous gems' respective main files with a if defined?(::ActiveRecord) and that would fix the issue with no side effects (so long as users have required Rails at the top of their Gemfile before AA, which is a universal practice)

@seanlinsley
Copy link

There are quite a few changes to Active Admin that haven't yet been released, including Rails 4 support and the move from meta_search to ransack.

@johnnyshields
Copy link
Contributor Author

Ransack looks like it has the same problem here:

https://github.com/ernie/ransack/blob/master/lib/ransack/adapters/active_record.rb I will raise a PR

@johnnyshields
Copy link
Contributor Author

PR raised for Ransack: activerecord-hackery/ransack#296

Ransack still has a dependency on polyamorous (see here: https://github.com/ernie/ransack/blob/master/ransack.gemspec)

@johnnyshields
Copy link
Contributor Author

@seanlinsley @ernie ORM agnosticism PR (activerecord-hackery/ransack#296) has been merged for Ransack (thanks @radar). May we please merge this one as well for Polyamorous?

@ernie ernie reopened this Nov 20, 2013
ernie added a commit that referenced this pull request Nov 20, 2013
Do not load if ActiveRecord not present.
Merged because @seanlinsley has special power over me.
@ernie ernie merged commit e80fb45 into activerecord-hackery:master Nov 20, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants