Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  pablobm February 15th, 2009 @ 06:27 PM

  Oh, blame Lighthouse's not providing a "preview" button. Let me try that piece of code again:

  
  c = Struct.new(:name).new('admin_space/product_line').extend(NameComposer) c.name # => "admin_space/product_line" c.underscored_name # => admin_space/product_line c.plural_base_name # => product_lines c.camelized_name # => AdminSpace::ProductLine
  

  The other two lines affected should be more or less readable anyway...

• You flagged this item as spam.
  

  pablobm February 18th, 2009 @ 08:52 PM

  • Title changed from “[patch] Scaffold generator not working well with namespaced resources” to “Scaffold generator not working well with namespaced resources”

  Updated patch file, as overly namespaced resources weren't being generated properly. Includes new test for this case.

  • fix-namespaced-scaffolds-generation.diff 45.2 KB
• 

  José Valim March 8th, 2009 @ 09:41 AM

  • Tag changed from “generator, master, patch, resource, scaffold” to “bug, generator, master, patch, resource, scaffold”

  Even with Admin::Something instead of admin/something it's not working?

• 

  Pratik March 9th, 2009 @ 12:57 PM

  • Assigned user set to “Pratik”

  Hey,

  Would it be possible to have the refactoring done in a separate ticket/patch if not too much work ?

  Thanks.

• You flagged this item as spam.
  

  pablobm March 15th, 2009 @ 03:27 AM

  @José: It doesn't not work with Admin::Something either.

  @Pratik: that makes sense but... I've been working on that today and have concluded that's a bit difficult to do. Or at least it's difficult that the patches make sense separately.

  All this boils down to the fact that the bits of code that make use of extract_modules() are very confusing. I did both things in parallel because:

  • If I first refactor and then fix the bug, the code after the first patch is still very confusing.

  • On the other hand, if I first fix the bug and then refactor... well, that's the scenario I wanted to avoid by refactoring in the first place.

  Does this sound reasonable?

• You flagged this item as spam.
  

  Dinshaw March 16th, 2009 @ 08:50 PM

  Sorry if this is a dumb question, but where is the code for the NameCompser?

• You flagged this item as spam.
  

  Dinshaw March 16th, 2009 @ 09:22 PM

  Ah.. got it. Updated patch doesn't have the name_composer.rb diff info. First one does. If that is how it was meant to be, sorry. I am new to patching and stuff.

• You flagged this item as spam.
  

  pablobm March 16th, 2009 @ 11:18 PM

  @Dinshaw: no, it's me the one who did it wrong. Thanks for pointing out :P Shit, I don't know why name_composer is not present in the second patch. I'm going to fix that now.

• You flagged this item as spam.
  

  pablobm March 16th, 2009 @ 11:52 PM

  Now, this is the correct patch. Sorry fellas, this was embarrasing...

  • 0001-Make-scaffold-and-resource-generators-work-nice-with.patch 50.2 KB

• You flagged this item as spam.
  

  blythe May 22nd, 2009 @ 07:36 PM

  +1

• You flagged this item as spam.
  

  pablobm May 30th, 2009 @ 03:29 AM

  • Tag changed from “bug, generator, master, patch, resource, scaffold” to “2-3-stable, 2.3.2.1, bug, generator, namespace, patch, resource, scaffold”

  Here goes another version of the patch, built upon v2.3.2.1

  This time it's composed of 11 smaller patches, each applying a single (and hopefully fairly simple) change to the code. All tests work after each single patch, and at the end the whole problem is solved.

  The first 9 patches undertake the refactoring mentioned on the original description.

  Patch number 10 adds new tests that show us (lo and behold) that the fix to this bug is halfway there after the initial refactoring.

  Patch number 11 fixes the generation of routes, which is the only item left before solving the issue.

  @Pratik: I hope this is satisfies your request. I didn't create a new ticket because I cannot quite see the refactoring as a ticket (the way I've done it at least), and then the fix as another ticket. Please correct me if wrong.

  • fixed-generation-of-namespaced-scaffolds-and-resources.patch 66.1 KB
• 

  CancelProfileIsBroken May 30th, 2009 @ 02:18 PM

  +1 - This area of the code is in need of some help, and this set of patches does the trick. I was able to create and use a namespaced scaffold after applying, which was not possible before. The refactoring should also make it possible to tackle some of the other issues in the scaffold generator.

  Patches also apply cleanly on master, and generator appears to work, but generated code isn't working under the current state of actionpack.

• 

  Pratik May 31st, 2009 @ 09:26 PM

  • Assigned user changed from “Pratik” to “Jeremy Kemper”
• 

  Michael Koziarski June 9th, 2009 @ 09:31 AM

  • Milestone changed from “2.x” to “2.3.4”

• You flagged this item as spam.
  

  kmpm July 22nd, 2009 @ 04:22 PM

  Have a look at #1628 which deals with fixtures and namespaces.

• You flagged this item as spam.
  

  pablobm July 22nd, 2009 @ 05:37 PM

  I haven't had a look at the patch at #1628 yet, and I'm not sure when I'll be as I just moved house and I still don't have a proper internet connection at my new place.

  However, I shall note that my patches don't generate namespaced models, and therefore don't generate namespaced fixtures either. The way I have chosen to implement it, admin/thing generates a model Thing, as opposed to Admin::Thing.

  This is because AR doesn't seem to automatically recognize admin_things as the appropriate table for an Admin::Thing model, so I decided to go for the simplest working solution. Whether this choice is appropriate or not, I'd love to hear.

• You flagged this item as spam.
  

  Kane July 22nd, 2009 @ 06:15 PM

  you generate a model without namespace if someone wants to generate one with namespace cause you think this is the simplest solution? LOL

• You flagged this item as spam.
  

  pablobm July 23rd, 2009 @ 09:49 AM

  I understand I might not have been entirely clear above, so I'll go into a bit of detail as to what the patches achieve or avoid to do, and why.

  After being patched, upon gently asking the scaffold generator to create "namespace/name", we get:

  • The namespaced controller "Namespace::NamesController" and test file at
  • app/controllers/namespace/names_controller.rb

  • test/functional/namespace/names_controller_test.rb

  • With routing to reach it at /namespace/names

  • Appropriate views at

  • app/views/namespace/names/index.html.erb
  • app/views/namespace/names/show.html.erb
  • app/views/namespace/names/new.html.erb

  • app/views/namespace/names/edit.html.erb

  • A layout at

  • app/views/layouts/namespace/names.html.erb

  • A helper "Namespace::NamesHelper" and a test file at

  • app/helpers/namespace/names_helper.rb

  • test/unit/helpers/namespace/names_helper_test.rb

  • The model "Name" with tests/fixtures/migration:

  • app/models/name.rb
  • test/unit/name_test.rb
  • test/fixtures/names.yml
  • db/migrate/20090723082416_create_names.rb

  And that's it.

  So, from the list above, we see the ONLY thing that is not namespaced is the MODEL. Controller/Views/Helper ARE namespaced, and can be accessed at a namespaced path /namespace/names through a namespaced route.

  Now, why didn't I namespace the model too? Reasons are:

  FIRST, ActiveRecord doesn't play nice with namespaced models. It expects Model "Namespace::Name" to have a table name of "name", instead of "namespace_name".
  The unpatched generator generated the model "Namespace::Name", with migrations/fixtures expecting it to work with a table "namespace_names". This wasn't working at all.

  SECOND, another course of action would have been patching AR too, but I wanted to restrict this to railties for now, before making it more complicated.
  I also could be adding "set_table_name" declarations to any namespaced model, but then that starts to turn ugly.
  So the SIMPLEST course of action was coming up with this solution. After that, feedback/debate here could think otherwise, and the patch could be changed.

  THIRD, personally, this patch scratches my itch. I normally create admin areas under /admin, where I manage models that are used in the public-facing side of the site. For me (and that's only me, I know) the models I work with in the Admin namespace are the same ones used in the rest of the site, and there is not much point in namespacing them at all.

  I hope it is all a little bit more clear now, and less laughable too, for that matter.

• You flagged this item as spam.
  

  pablobm July 23rd, 2009 @ 09:49 AM

  Crap, lighthouse messing up with the formatting again. Anyway...

• You flagged this item as spam.
  

  Kane July 23rd, 2009 @ 04:34 PM

  Okey thanks for the clarification, i often work with namespaces, for models too.
  I would vote for a seperate patch that fixes the ActiveRecord behaviour, so the generators can be patched accordantly.

• You flagged this item as spam.
  

  Kane July 23rd, 2009 @ 11:09 PM

  the behaviour of 2.3.2 is really interesting..

  ruby script/generate model Site::Article text:text
  ruby script/generate model site/article text:text

  works both which corresponds with the doc
  "Pass the model name, either CamelCased or under_scored"

  but it creates a subfolder and doesnt put the fixtures in it:

  create test/fixtures/site
  create test/fixtures/site_articles.yml

  and to the tablenames..
  its really strange in my opinion

  class CreditCard < ActiveRecord::Base
    class PinNumber < ActiveRecord::Base
    end
  end
  
  assert_equal "credit_card_pin_numbers", CreditCard::PinNumber.table_name
  
  module Billing
    class Account < ActiveRecord::Base
    end
  end
  
  assert_equal 'accounts', Billing::Account.table_name
  

  and it seems this is intended behaviour, i took this from the test cases.
  base_test.rb and modules_test.rb

  can someone explain why the module should not go into the tablename?

• 

  Michael Koziarski July 23rd, 2009 @ 11:22 PM

  The module doesn't go in the table name as it's intended 'just' for
  organisational purposes.

  Nesting the class implies ownership, so the table name is prefixed.

  This is intentional.

• You flagged this item as spam.
  

  Kane July 24th, 2009 @ 05:49 PM

  so the model generator shouldnt build the namespace into the tablename.
  Ive made a patch which adds a test for namespaced model generation and verifies that the migration creats the table correctly.
  I fixed also the mentioned bug with fixture creation.

  • fix_generator.diff 2.9 KB
• 

  José Valim July 27th, 2009 @ 12:20 PM

  Koz, can I handle this for Rails 3.0?

  1) No namespaces in the table name
  2) Namespace in fixtures directory

  Is it the right way to go?

• You flagged this item as spam.
  

  kmpm July 28th, 2009 @ 07:33 AM

  Kane, your fixed generator looks ok but it can't load it's fixtures neither in development or when runing unit tests.

• You flagged this item as spam.
  

  Kane July 28th, 2009 @ 07:37 PM

  shouldnt be too much work?

  fixtures :all should also load subdirectories

  fixtures :table_name has to find a class with this tablename.
  I assume this is trying to find a class named table_name.singularize. only have to make him to allow namespace before it.

  after this and my patch pablobm can update his patch and the whole story should be history.

  kmpm, i looked at #2965. I will use your tests and look into it.

• You flagged this item as spam.
  

  Kane July 28th, 2009 @ 07:44 PM

  jose:

  1) is a two line delete.

  2) i assume if namespaces for models are for organisational purpose and are subdirs in /models, in case of the corresponding fixtures it should be the same. it would be inconsistent if a model is in models/namespace/ but the fixture is in fixtures/.

• 

  José Valim August 10th, 2009 @ 10:22 AM

  • Assigned user changed from “Jeremy Kemper” to “José Valim”
  • Milestone cleared.

  Kane, we need first to solve the fixtures issue before applying the patch. Have you had any success in that front?

• You flagged this item as spam.
  

  Kane August 13th, 2009 @ 12:29 AM

  i had a look at it. the underlying problem is that the fixture code handles everything through the tablename, and so we have no clue about the namespace cause the only thing that specifies it is the folder in which the fixture is placed.

  another problem is the following scenario if we specify the fixture in the test only with the tablename we can only gamble which fixture file is meant.

  /fixtures/namespace_a/articles.yml

  /fixtures/namespace_b/articles.yml

  class NamespaceA::Article < ActiveRecord::Base
  end
  
  class NamespaceB::Article < ActiveRecord::Base
  end
  
  class WhateverTest < ActiveSupport::TestCase
    fixtures :articles
  end
  

  So we have to rethink this.
  There seems to be no way to avoid generating a set_fixture_class line.

  fixtures :all shouldnt be a problem so it just have to fill both fixture files in the table.

  BUT: what to do if we want to get an article in our tests?
  articles(:first).find has also no chance to detect which model it should use.

• You flagged this item as spam.
  

  kmpm August 13th, 2009 @ 05:53 AM

  Kane... In #2965 I also mention the fact that when doing rake db:fixtures:load there isn't a way to do set_fixture_class so I really don't see that as a good workaround.

• You flagged this item as spam.
  

  Kane August 15th, 2009 @ 08:10 PM

  worked on the patch and it looks good, but:
  where are the tests for rake tasks or how to test db:fixtures:load?

• 

  José Valim August 15th, 2009 @ 08:53 PM

  The rake tasks usually just invoke a method in a class. The method is properly tested on its framework (for example, fixtures tests on ActiveRecord). And can you please add the patch to another ticket? #2965 already has a patch with tests that you might want to take a look, if it's the same scenario, you could attach it there. :)

• You flagged this item as spam.
  

  sebwin September 16th, 2009 @ 01:26 PM

  Well, I really think it would have been nice if, for the time being, pablobm's patch had been incorporated in order to make namespaced scaffolding work as, I would guess, 95% of the users expect it to.

• 

  José Valim March 6th, 2010 @ 07:20 AM

  • Milestone cleared.
• 

  José Valim March 12th, 2010 @ 08:35 PM

  • State changed from “new” to “stale”

  Marking as stale. If this is still a problem or someone is working on it, I will be glad to reopen.