Fix engine's generator

[flippingbits]

I've moved load_generator from Rails::Application to Rails::Engine in order to provide generators for Rails engines (fixes #1259).

I've introduced rails/engine/commands that provides engine specific commands.
Furthermore, I've done some small cleanups and improvements.

Regards

[josevalim]

This looks good. However, I don't think we should always generate an engine file. I think that should happen only with the full option. What we could (and need to) do is to generate script/rails on rails plugin new only if the engine is full, so we can guarantee we won't have issues. Also, I think we can tidy up rails/commands/application after those commits. Finally, don't forget to add the destroy command as well (the inverse of generate).

[josevalim]

Summoning @drogus to join the discussion!

[drogus]

this can be DRYed up a bit:

root = defined?(ENGINE_ROOT) ? ENGINE_ROOT : Rails.root
Rails::Generators.invoke name, ARGV, :behavior => :invoke, :destination_root => root

[flippingbits]

Thanks, I'll refactor it :)

[flippingbits]

Yes, I was struggling if we should always generate an engine file or not. IMHO we should, because then generators will be available in every engine, whether it's full or not.
If you don't agree with this, I'm fine with reverting/removing the concerning commit.

[drogus]

@flippingbits The thing is that generator can also create bare rails plugin or railtie. I think that we should generate it for options that generate engine. Otherwise it will break as @josevalim mentioned.

The other option would be to alway generate it, but add nice warning if there is no engine when you try to run it and also add option to be able to skip it. But the first option seems much simpler.

[josevalim]

@flippingbits plugin new is also used to generate simpler plugins, that for example, just extend ActiveRecord without an app directory in the first place. In such cases, generators (or any of the command line options) are not really needed.

[flippingbits]

That's fine for me. So, I'll add the engine and script/rails only if --full is given.

[josevalim]

@flippingbits I thought this was the original behavior in the first place. :) @drogus, can you remember why we were shipping with script/rails for everything? Was it intentional?

[flippingbits]

@josevalim @drogus IMHO, script/rails is only needed by full engines.
With the last commit, only full engines will get a script/rails file and therefore access to Rails generators.

[josevalim]

@flippingbits agreed. I am just wondering if there is a reason for that to not be happening now in the first place. Those commits look good. If @drogus agree with me, could you also add support to "script/rails destroy" so we can finally merge it?

[flippingbits]

script/rails destroy has been added 3 minutes ago :)

[flippingbits]

Do you think we can get this merged into 3-1-stable and therefore into the 3.1.0 release? IMHO it's a must-have.
BTW, if you have any desire for corrections, I'm happy to help :)

[josevalim]

We should not merge on 3-1-stable. @drogus, is this good for master?

José Valim
www.plataformatec.com.br
Founder and Lead Developer
*

[drogus]

Sorry for delayed answer. This is overally ok, but from my quick tests, it breaks generators from isolated engines. Try adding:

module Blog
  class Engine < ::Rails::Engine
    isolate_namespace Blog
  end
end

and running rails g scaffold post title:string body:text. Ideally it should generate Blog::Post model, unless you provide --skip-namespace.

[drogus]

There is also another problem. In engine with dummy app, there are some rake tasks that should work with integration wit dummy path (like db tasks). This was added to allow people develop engines similarly to applications, so you can do something like that:

rails g scaffold post title:string body:text
rake db:migrate # runs migrations for dummy app + engine, so you can test it properly

Unfortunately this was not tested properly, but there should be such tests, at least for simplest cases (like rake db:migrate).

Also, namespaced generators does not work because of a hack that I used to not rewrite the entire generators system ;)
https://github.com/rails/rails/blob/master/railties/lib/rails/generators/named_base.rb#L65

[josevalim]

@flippingbits if you want to tackle those missing parts, please let me know and I will guide you. If not, I will try to find some time in the next weeks to tidy this up.

[flippingbits]

@drogus Thanks for your feedback!

@josevalim I'd love to do the missing tasks. Where can we talk?

[radar]

It appears to me that when I generate a controller using @flippingbits branch of Rails it still isn't namespaced. Perhaps I am doing it wrong? Is there an easier way I can confirm this?

$ rails g controller topics
create  app/controllers/topics_controller.rb
invoke  erb
create    app/views/topics
invoke  test_unit
create    test/functional/topics_controller_test.rb
invoke  helper
create    app/helpers/topics_helper.rb
invoke    test_unit
create      test/unit/helpers/topics_helper_test.rb
invoke  assets
create    app/assets/javascripts/topics.js
invoke    css
create      app/assets/stylesheets/topics.css

[radar]

Ah, I just saw drogus's reply earlier. Apparently it's still broken.

[radar]

Stefan and I have talked on Jabber about this and I have come up with a starting point for this: https://github.com/radar/rails/blob/master/railties/test/engine/generators_test.rb. I don't know much about the practices of testing in Rails and so I have just gone with the "whatever works" development pattern.

I strongly believe that Rails 3.1 should not be released until this issue is completely solved. If you have a precise reason why you would rather release it with something as critical as generators within an engine still being broken, let me know. I am curious.

[josevalim]

@radar Are you sure this issue also exists on Rails 3.1? For me it was happening just on master. If that is the case, I will happily apply that previous patch from you. Do you still have it around?

[drogus]

@flippingbits: When you will be fixing it, you can take a look at tests that @radar did to reproduce this: #1483 (unless you already have that cases added as we discussed)

[flippingbits]

I'll do. My current solution is pretty similar to radar's, so I'll take his as base.

[flippingbits]

I've added a sneak preview, more tests are coming :)

[josevalim]

It looks good! Just one small suggestion: maybe we could use most of the contents in https://github.com/rails/rails/blob/master/railties/test/isolation/abstract_unit.rb instead of doing everything manually in test/engine/generators_test.rb? Also, feel free to place the generators tests inside test/railties/generators_test.rb. We usually place everything engine related inside test/railties. :)

[flippingbits]

@josevalim requiring https://github.com/rails/rails/blob/master/railties/test/isolation/abstract_unit.rb creates always a new Rails application. IMHO, we don't need this behavior for testing generators.

[josevalim]

Yeah, but you could something like this:

RAILS_ISOLATION_COMMAND = "engine"
require "isolation/abstract_unit"

And then, the isolation file could check for RAILS_ISOLATION_COMMAND and generate things accordingly. I know using constants as signal is bad, but at least we would be able to re-use the existing structure. Wdyt?

[flippingbits]

Yes, this might be a solution, I'll push it later.
Do you think that we need to test anything else regarding generators or can we continue with testing rake tasks?

[josevalim]

Don't worry about adding a complete suite. Just add a regression for the case you are fixing right now and we are done. :)

[josevalim]

We don't need this default implementation anymore. It could simply be "mattr_accessor :namespace" :)

[flippingbits]

I've fixed other tests from the railties test suite.

BTW, Rails definitely needs CI :)

[radar]

It has a CI, just not many people know about it: http://ci.rubyonrails.org/

[josevalim]

@flippingbits is this ready to merge? :D

[flippingbits]

I think so :)