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

Fix engine's generators #1327

Closed
wants to merge 3 commits into from

Conversation

flippingbits
Copy link

Hi all,

this is a fix for Rails generators inside an engine. They currently depend on the dummy app that breaks them if no dummy app exists (e.g. if test unit files are skipped).

I've extracted the generators and added a new class Application inside the lib folder, that provides a Rails::Application but doesn't get loaded by Rails applications that use the engine.

This fixes issue #1259.

Regards

@josevalim
Copy link
Contributor

Hrm, this is a hack around the current bug. Probably the best way to fix is this is by moving the load_generators code from Application to Engine and call it on the engine. We could probably do the same for console and make rails console also work for engines.

@flippingbits
Copy link
Author

Sure, I'll do some work on this topic.

@kristianmandrup
Copy link

"Probably the best way to fix is this is by moving the load_generators code from Application to Engine and call it on the engine. We could probably do the same for console and make rails console also work for engines."

Yeah, sounds like a nice solution! BTW, any plans to enable running executables such as bundle and rails within a dummy application in a project?

Currently this doesn't work, hence it's complicated to set up a dummy app for testing purposes - you first have to export it out, run your bundle/rails commands, then import the project back into your test folder :(

https://github.com/kristianmandrup/multiapp-engine

@flippingbits
Copy link
Author

IMHO, the only commands we need inside an engine's project are generate and console, as @josevalim already pointed out.
rails/commands is pretty hard to extend to achieve this behavior.
What do you think about introducing rails/engine/commands that handles the commands that are needed by engines only?
This solution separates application's and engine's command line processing.

@kristianmandrup
Copy link

Sounds good for now. Let's try it out and see how it works in real life scenarios. I would however like to be able to run the bundler commands inside the spec folder, fx to configure a dummy rails app with some other gems in order to see how my engine plays in an app configured with other gems (integration testing).

@flippingbits
Copy link
Author

The bundler commands aren't provided by Rails and shouldn't be affected.
The only thing I'd do is providing a clean command set for engines.

@josevalim
Copy link
Contributor

I dont think we need commands for engines. We partially handle the engine case in the current commands file, I just think we need to wrap it somewhere.

@flippingbits
Copy link
Author

Hm, handling all together in one file would be the best solution. But the commands are totally different.
E.g. we need one help command for engines and one for regular Rails applications.
They've only one thing in common: the processing of command aliases.

@josevalim
Copy link
Contributor

As you seem fit sir.

@flippingbits
Copy link
Author

Hm? :)

@josevalim
Copy link
Contributor

If you think it is better to have two different files, gO ahead and try it. Code >>> discussion.

@flippingbits
Copy link
Author

Indeed, I'll do.

@drogus
Copy link
Member

drogus commented May 27, 2011

@flippingbits: for the record, we've tried the "single file application" approach, but this is not optimal and can lead to subtle bugs. Think about that: if you set some config options for an Engine and you leave Application file untouched, how will generators now that they need to us Engine's options?

@flippingbits
Copy link
Author

Yep, good point, @drogus!

I've successfully moved load_generators from Application to Engine and modified the commands by creating rails/engine/commands.
I'll create a push request later, where we can discuss this solution. Or should we continue discussing here?

@flippingbits
Copy link
Author

The new pull request is available: #1356.

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.

None yet

4 participants