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 ActiveAdmin ORM agnostic (i.e. works without loading ActiveRecord) #2545

Merged
merged 2 commits into from
Oct 9, 2013

Conversation

johnnyshields
Copy link
Contributor

  1. Add ORM-specific loaders:
  • lib\active_admin\orm\active_record
  • lib\active_admin\orm\mongoid
  1. Move Comments plugin to ActiveRecord ORM directory. Comments support is AR-only for now.

  2. Add ActiveRecord/Mongoid type checks to DisplayHelper#pretty_format(object) method

- lib\active_admin\orm\active_record
- lib\active_admin\orm\mongoid

Move comments to ActiveRecord ORM directory

Add ActiveRecord/Mongoid type checks to DisplayHelper#pretty_format(object) method
require 'active_admin/batch_actions'
require 'active_admin/filters'

# Require ORM-specific plugins
require 'active_admin/orm/active_record' if defined?(::ActiveRecord)
require 'active_admin/orm/mongoid' if defined?(::Mongoid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if both ActiveRecord and Mongoid are present?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, both will load. As long as the plugins within the directories don't conflict with each other, should not be a problem. (Currently there are no Mongoid plugins, so no conflicts). We could add a global config option to AA so user can control which set(s) to load.

@johnnyshields
Copy link
Contributor Author

@daxter committed change as per your request

@seanlinsley
Copy link
Contributor

No, the problem was one of indentation. The code is currently like this:

case foo
  when Bar
    baz
end

When it should be like this:

case foo
when Bar
  baz
end

There was nothing wrong with the nested if statement.

@johnnyshields
Copy link
Contributor Author

Ah oops. Well, I've now fixed the indentation and I've kept the procs because it looks nicer anyway.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling 94d74ab on johnnyshields:orm-agnosticism into 75de0f1 on gregbell:master.

@seanlinsley
Copy link
Contributor

Okay, looks good. Thanks for taking the time to contribute.

seanlinsley added a commit that referenced this pull request Oct 9, 2013
Make ActiveAdmin ORM agnostic (i.e. works without loading ActiveRecord)
@seanlinsley seanlinsley merged commit cd424fc into activeadmin:master Oct 9, 2013
@fred
Copy link

fred commented Apr 9, 2014

Hi,
Master is still trying to load ActiveRecord.
I use mongoid and dont have "active_record/railtie" loaded in application.rb

any ideas how to solve this??

/Users/fred/.rvm/gems/ruby-2.1.1/bundler/gems/active_admin-8ea922bb6df5/lib/active_admin/application.rb:180:in `rescue in load': uninitialized constant ActiveAdmin::Application::ActiveRecord (NameError)
    from /Users/fred/.rvm/gems/ruby-2.1.1/bundler/gems/active_admin-8ea922bb6df5/lib/active_admin/application.rb:179:in `load'
    from /Users/fred/.rvm/gems/ruby-2.1.1/bundler/gems/active_admin-8ea922bb6df5/lib/active_admin/application.rb:171:in `block in load!'
    from /Users/fred/.rvm/gems/ruby-2.1.1/bundler/gems/active_admin-8ea922bb6df5/lib/active_admin/application.rb:171:in `each'
    from /Users/fred/.rvm/gems/ruby-2.1.1/bundler/gems/active_admin-8ea922bb6df5/lib/active_admin/application.rb:171:in `load!'
    from /Users/fred/.rvm/gems/ruby-2.1.1/bundler/gems/active_admin-8ea922bb6df5/lib/active_admin/application.rb:195:in `routes'
    from /Users/fred/.rvm/gems/ruby-2.1.1/bundler/gems/active_admin-8ea922bb6df5/lib/active_admin.rb:80:in `routes'

@johnnyshields
Copy link
Contributor Author

@fred there was probably a code change done since my original commit. You'll have to raise a new PR and put a if defined?(::ActiveRecord) block around the offending code.

@seanlinsley
Copy link
Contributor

It'd be nice to have a test or two to confirm this doesn't happen in the future, but the test suite is already so slow :-/

@johnnyshields
Copy link
Contributor Author

Its simple to do--just set hide_const('ActiveRecord') / hide_const('ActiveRecord::Base') and boot the app.

@fred
Copy link

fred commented Apr 10, 2014

Thanks @johnnyshields hope to the pull request in.

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.

5 participants