Skip to content

Feature/engine support (Issue #11) #12

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rthbound
Copy link

@rthbound rthbound commented Jan 7, 2013

Addresses #11 by generating an env.rb file that loads routes for any mounted engine and for the application itself (loads url_helpers for main_app and 'engine_name')

Warning, it's ugly... The thought of this code going into every spinach user's env.rb file grosses me out.

Perhaps it could be moved into the railties file...

@josepjaume
Copy link
Contributor

This is really cool :)

@@ -9,6 +9,28 @@ def create_environment_file

require 'minitest/spec'

module Spinach
class FeatureSteps
known_engines = ObjectSpace.enum_for(:each_object, ::Rails::Railtie.singleton_class).to_a
Copy link
Contributor

Choose a reason for hiding this comment

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

This hack is pretty neat, but isn't there any other way to iterate through all the engines other than that? Also, I think this only works if spinach is loaded after all the other Engines am I right?

If that's the case, maybe this code should be initialized someplace here:

https://github.com/codegram/spinach-rails/blob/master/lib/spinach-rails/railtie.rb#L12

There should be a way to be sure we're the last gem to be initialized.

Or did I get something wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see you actually mentioned this on the Issue 👍

Do you have the time to maybe tackle the problem from the Railtie and we can then merge it into master?

Copy link
Author

Choose a reason for hiding this comment

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

Would love to... Maybe you also noticed the engine is installed into vendor/plugins, support for which will be removed in Rails 4. May want to address that prior to merging, also.

Copy link
Member

Choose a reason for hiding this comment

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

@rthbound ObjectSpace is disabled by default in JRuby. I don't think it's a very good idea to use it if we plan to keep our cross-implementation support.

Copy link
Author

Choose a reason for hiding this comment

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

@txus
Totally fair. It's a handy hack, but agreeably not a solution we should merge.

Copy link
Member

Choose a reason for hiding this comment

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

If you get rid of ObjectSpace, it's a big +1 for me and I'll merge it right away :)

Copy link
Author

Choose a reason for hiding this comment

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

Looks like Rails::Engine.ancestors could replace searching ObjectSpace

@rthbound
Copy link
Author

@txus I've got something together, but tests are failing (even on a fresh clone)...

@txus
Copy link
Member

txus commented Dec 20, 2013

@rthbound any luck?

@rthbound
Copy link
Author

@txus looks like the failures are due to problems with the test itself and likely not to do with the implementation. Have not yet confirmed.

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.

3 participants