Fix engine's generator #1356

Merged
merged 22 commits into from Jun 9, 2011

Conversation

Projects
None yet
4 participants
Contributor

flippingbits commented May 27, 2011

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

Contributor

josevalim commented May 27, 2011

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).

Contributor

josevalim commented May 27, 2011

Summoning @drogus to join the discussion!

railties/lib/rails/commands/generate.rb
@@ -7,4 +7,9 @@ if ARGV.first.in?([nil, "-h", "--help"])
end
name = ARGV.shift
-Rails::Generators.invoke name, ARGV, :behavior => :invoke, :destination_root => Rails.root
+
+if defined?(ENGINE_ROOT)
@drogus

drogus May 27, 2011

Member

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

flippingbits May 27, 2011

Contributor

Thanks, I'll refactor it :)

Contributor

flippingbits commented May 27, 2011

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.

Member

drogus commented May 27, 2011

@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.

Contributor

josevalim commented May 27, 2011

@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.

Contributor

flippingbits commented May 27, 2011

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

Contributor

josevalim commented May 27, 2011

@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?

Contributor

flippingbits commented May 27, 2011

@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.

Contributor

josevalim commented May 27, 2011

@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?

Contributor

flippingbits commented May 27, 2011

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

Contributor

flippingbits commented May 28, 2011

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 :)

Contributor

josevalim commented May 28, 2011

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

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

Member

drogus commented May 31, 2011

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.

Member

drogus commented May 31, 2011

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

Contributor

josevalim commented May 31, 2011

@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.

Contributor

flippingbits commented May 31, 2011

@drogus Thanks for your feedback!

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

Contributor

radar commented Jun 6, 2011

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
Contributor

radar commented Jun 6, 2011

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

Contributor

flippingbits commented Jun 6, 2011

:) Yes, it's still broken. I'm working on it!

Contributor

radar commented Jun 6, 2011

Anything I can do to help / speed this up other than kidnapping you?

Contributor

flippingbits commented Jun 6, 2011

I'm currently working on improving the general test suite regarding engine's generators and rake tasks.
We can talk in Jabber, if you want to?

Contributor

radar commented Jun 6, 2011

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.

Contributor

josevalim commented Jun 6, 2011

@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?

Member

drogus commented Jun 7, 2011

@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)

Contributor

flippingbits commented Jun 7, 2011

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

Contributor

flippingbits commented Jun 7, 2011

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

Contributor

josevalim commented Jun 7, 2011

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. :)

Contributor

flippingbits commented Jun 7, 2011

@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.

Contributor

josevalim commented Jun 7, 2011

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?

Contributor

flippingbits commented Jun 7, 2011

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?

Contributor

josevalim commented Jun 7, 2011

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

railties/lib/rails/generators.rb
@@ -90,6 +90,16 @@ module Rails
@options ||= DEFAULT_OPTIONS.dup
end
+ def self.namespace
@josevalim

josevalim Jun 7, 2011

Contributor

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

Contributor

flippingbits commented Jun 7, 2011

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

BTW, Rails definitely needs CI :)

Contributor

radar commented Jun 9, 2011

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

Contributor

josevalim commented Jun 9, 2011

@flippingbits is this ready to merge? :D

Contributor

flippingbits commented Jun 9, 2011

I think so :)

josevalim added a commit that referenced this pull request Jun 9, 2011

@josevalim josevalim merged commit eb8c0a7 into rails:master Jun 9, 2011

jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011

Ensure shallow routes respects namespace [#1356 state:resolved]
Signed-off-by: Pratik Naik <pratiknaik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment