Hi James, hi all, Like we agreed on #engines last week I've worked on Engines for Rails 2.0 (still using RC1) this weekend. I've commited my work here: http://svn.artweb-design.de/engines/tests/rails_2_0 Please criticize everything :) Analog to the test apps in the Engines repo this is a complete rails app. You should be able to run the tests once you've created the database(s). Apperently the migration tests uses the development database at the moment when you use rake test:units. (I'll have to figure out how to only rely on the test database, TODO) Based on our discussion on IRC I've added tests and further refactored the Engines plugin. As for the tests I had to invest some time to figure out how Al Pacino, Baldwin, Stephen and William work together with James and how they all cope with funk, bananas and hobbits! (Which was quite fun.) So I've decided to restructure this using some more descriptive names ... hoping to increase comprehensibility a little. I hope this is ok for you. Also, doing this I think I was able to remove some duplicate and add some missing functionality to test. I believe that the tests now cover the important parts (not sure though) aside from these (TODO): * ActionMailer breaks with the Engines::RailsExtension (tests disabled) * $LOAD_PATH seems to be in reverse order so tests fail here (tests disabled) * no tests for arbitrary code mixing ported (unclear what's going to drop) As for the refactoring, I've changed quite a bit (mostly but not solely responding to your considerations in IRC): * moved Rails.plugins to Engines.plugins * removed about.yml and version attribute * removed Engines.current * removed block from Engines::PluginList, left the rest there for now * removed LegacySupport * added an Engines::Assets module to hold file/dir creation/copying related stuff * added the following line in Engines::RailsExtensions::Routing map = self # allows to just copy and paste routes around * moved extensions to an accessible attribute on Engines and extensions loding from init.rb to Engines.init to declutter the init.rb file * supposedly quite some stuff that I forgot to take notes about ... Engines and Engines::Plugin are pretty lean now. I moved stuff out of init.rb because I thought it would be a nice idea to have the Engines plugin behave just like a normal Rails plugin if it hasn't been "booted". I think this also responds to your consideration to mess as little with Rails core code as possible. (On the other hand) I've added a default_attr_accessor method to Class in order to dry up the repetition in Engine::Plugin. Personally I feel that this extension to Class is quite useful (it could even go into ActiveSupport). If you don't like it, I'll revert this. Regarding the location of the routes.rb file: I'm sure this has been discussed before, but as there are the plugin subdirectories /db/migrations and / public ... why isn't the directory structure mirrored for the routes file? I.e. why is it located in the plugin root dir instead of a /config subdir? IMHO mirroring the directory structure here would make things even more intuitive. -- sven fuchs svenfuchs@artweb-design.de artweb design http://www.artweb-design.de grünberger 65 + 49 (0) 30 - 47 98 69 96 (phone) d-10245 berlin + 49 (0) 171 - 35 20 38 4 (mobile)
on 02.12.2007 17:07
on 02.12.2007 21:04
On Dec 2, 2007, at 8:06 AM, Sven Fuchs wrote: > Analog to the test apps in the Engines repo this is a complete rails > Engines plugin. > you. > * $LOAD_PATH seems to be in reverse order so tests fail here (tests > disabled) Did you follow this discussion on http://groups.google.com/group/rubyonrails-core/browse_thread/thread/5f17c2a87a122613#320622ec48b0129a ? What's the cause of the failure? Cheers, Pascal. -- http://blog.nanorails.com
on 02.12.2007 21:17
Woha, folks, I appologize for the awkward formatting of my last mail ... I shouldn't have just pasted the text from Textmate :( Am 02.12.2007 um 21:04 schrieb Pascal: > On Dec 2, 2007, at 8:06 AM, Sven Fuchs wrote: >> >> * $LOAD_PATH seems to be in reverse order so tests fail here (tests >> disabled) > Did you follow this discussion on http://groups.google.com/group/rubyonrails-core/browse_thread/thread/5f17c2a87a122613#320622ec48b0129a > ? Not quite. I was aware that it's there ... but I haven't been able to really follow it. I'll try to catch up on that. > What's the cause of the failure? I'm not sure, yet. It might be caused by my own changes to Engines, some changes from Rails 1.2 to Rails 2.0 RC1 (which I used) or even a bug in my testcases. I'm not even totally sure if I really understand how things are supposed to work here. Can you give me a recap? -- sven fuchs svenfuchs@artweb-design.de artweb design http://www.artweb-design.de grünberger 65 + 49 (0) 30 - 47 98 69 96 (phone) d-10245 berlin + 49 (0) 171 - 35 20 38 4 (mobile)
on 02.12.2007 21:46
On Dec 2, 2007, at 12:17 PM, Sven Fuchs wrote: > > supposed to work here. > > Can you give me a recap? Comments 11 Pascal and 13 James Adam should give you an idea of what's going on and possible issues with that. Basically, $LOAD_PATH is in reverse order to give plugins initialized later more weight. The problem is that if the path to a file is used more than once, the first one gets totally hidden. Possible solutions to ensure a plugin loads its code and nothing else in the load path: - use absolute paths init.rb to require all the files require File.join(File.dirname(__FILE__), 'lib/filtered_column/ processor') require File.join(File.dirname(__FILE__), 'lib/filtered_column/mixin') require File.join(File.dirname(__FILE__), 'lib/filtered_column/filters/ base') require File.join(File.dirname(__FILE__), 'lib/filtered_column/macros/ base') - insert your lib in front of everyon in init.rb $:.unshift(File.dirname(__FILE__) + '/lib') the second solution is easier, but can introduce a whole new set of loading problems for other plugins since you are changing the load path. Cheers, Pascal. -- http://blog.nanorails.com
on 02.12.2007 22:53
Thanks for all the pointers, Pascal. After reading the thread I now think that the problem is simply in my testcase as it's based my former assumption that the plugin's load_paths should appear in the order in that they were loaded/ initialized. Am 02.12.2007 um 21:45 schrieb Pascal: >>>> disabled) >> bug in my testcases. > > macros/base') > -- >> > http://lists.rails-engines.org/listinfo.cgi/engine-developers-rails-engines.org -- sven fuchs svenfuchs@artweb-design.de artweb design http://www.artweb-design.de grünberger 65 + 49 (0) 30 - 47 98 69 96 (phone) d-10245 berlin + 49 (0) 171 - 35 20 38 4 (mobile)
on 02.12.2007 23:17
On Dec 2, 2007 4:06 PM, Sven Fuchs <svenfuchs@artweb-design.de> wrote: > Hi James, hi all, Hi Sven, > Like we agreed on #engines last week I've worked on Engines for Rails 2.0 > (still using RC1) this weekend. I've commited my work here: > > http://svn.artweb-design.de/engines/tests/rails_2_0 I've checked out your code, and I'll be looking over it this week :) > As for the tests I had to invest some time to figure out how Al Pacino, > Baldwin, Stephen and William work together with James and how they all > cope with funk, bananas and hobbits! (Which was quite fun.) > > So I've decided to restructure this using some more descriptive names ... > hoping to increase comprehensibility a little. I hope this is ok for you. That seems fair :) > * ActionMailer breaks with the Engines::RailsExtension (tests disabled) > * $LOAD_PATH seems to be in reverse order so tests fail here (tests > disabled) > * no tests for arbitrary code mixing ported (unclear what's going to > drop) I believe the rendering mechanism for ActionMailer has changed, I'll investigate this. See Pascal's comments about the $LOAD_PATH - I believe it is in the correct order. I'm not sure that your test for this makes sense, but I'll double check this. While code mixing might be deprecated, it could be worth keeping parts of it around - for instance, I'm quite keen on moving the explicit declaration of "controllers and helpers" out of the depths of the Dependencies extensions to somewhere more visible. > As for the refactoring, I've changed quite a bit (mostly but not solely > responding to your considerations in IRC): > > * moved Rails.plugins to Engines.plugins This could be problematic, as existing migrations will refer to Rails.plugins. I suggest that we add a method to the Rails module which returns the result of a call to Engines.plugins. > * removed about.yml and version attribute > * removed Engines.current > * removed block from Engines::PluginList, left the rest there for now > * removed LegacySupport > * added an Engines::Assets module to hold file/dir creation/copying > related stuff > * added the following line in Engines::RailsExtensions::Routing > map = self # allows to just copy and paste routes around These all seem fine to me. > * moved extensions to an accessible attribute on Engines and > extensions loding from init.rb to Engines.init to declutter the init.rb file. > Engines and Engines::Plugin are pretty lean now. I moved stuff out of > init.rb because I thought it would be a nice idea to have the Engines plugin > behave just like a normal Rails plugin if it hasn't been "booted". I think this > also responds to your consideration to mess as little with Rails core code as > possible. I'm not sure if I follow you here - by 'not booted', presumably you mean that the line hasn't been added to the top of environment.rb? > (On the other hand) I've added a default_attr_accessor method to Class > in order to dry up the repetition in Engine::Plugin. Personally I feel > that this extension to Class is quite useful (it could even go into > ActiveSupport). If you don't like it, I'll revert this. I sympathise, but I think it's possibly wise to avoid messing around with Ruby core classes unless it's unavoidable. I'd be more comfortable setting the defaults in a more explicit way. > Regarding the location of the routes.rb file: > ... why isn't the directory structure mirrored for the routes file? > I.e. why is it located in the plugin root dir instead of a /config subdir? IMHO > mirroring the directory structure here would make things even more > intuitive. The distinction here is that the only file in our "config" directory would be the routes.rb file. Is it really worth creating a directory to hold that single file? Personally, I don't think so. I also feel that it's worth breaking away from the "your plugin is a mini application" mindset that mirroring the default app layout so completely would promote. Anyway - I need to spend some time merging what you've done into my own refactorings, and then I'm sure I'll have more to add. Thanks for your work so far Sven! Let me know if you have any other ideas,
on 03.12.2007 01:17
Hi James, I've commited everything to the Engines repo now: http://svn.rails-engines.org/test/engines/rails_2.0/ Thanks for granting me commit access! Am 02.12.2007 um 23:16 schrieb James Adam: > I believe the rendering mechanism for ActionMailer has changed, I'll > investigate this. Great. > See Pascal's comments about the $LOAD_PATH - I > believe it is in the correct order. I'm not sure that your test for > this makes sense, but I'll double check this. Yes, I'm pretty sure my test is just plain crap. I'll change that. > While code mixing might be deprecated, it could be worth keeping parts > of it around - for instance, I'm quite keen on moving the explicit > declaration of "controllers and helpers" out of the depths of the > Dependencies extensions to somewhere more visible. Aha, ok. Well, I've put porting the tests for that on my todo list. >> * moved Rails.plugins to Engines.plugins > This could be problematic, as existing migrations will refer to > Rails.plugins. I suggest that we add a method to the Rails module > which returns the result of a call to Engines.plugins. Ok. >> possible. > I'm not sure if I follow you here - by 'not booted', presumably you > mean that the line hasn't been added to the top of environment.rb? Yes. "booted" because it happens right after Rails' own boot.rb has finished ... i.e. just "present" or "included". I might be wrong but I think previously you'd had to remove the entire Engines plugin from the plugins directory to deactivate it. Or comment out everything in the init.rb file? I'm not sure if it's actually a big win to be able to deactivate it by commenting out the line in environment but it feels a little cleaner (ignoring the fact that it doesn't really work that way when there are map.from_route mappings in routes.rb). > I sympathise, but I think it's possibly wise to avoid messing around > with Ruby core classes unless it's unavoidable. I'd be more > comfortable setting the defaults in a more explicit way. Ok, I'll revert that. > The distinction here is that the only file in our "config" directory > would be the routes.rb file. Is it really worth creating a directory > to hold that single file? Personally, I don't think so. I also feel > that it's worth breaking away from the "your plugin is a mini > application" mindset that mirroring the default app layout so > completely would promote. Hmm, I guess that's a matter of taste probably. Ok, strike that. > Anyway - I need to spend some time merging what you've done into my > own refactorings, and then I'm sure I'll have more to add. Thanks for > your work so far Sven! It's a lot of fun, actually. And I'm happy to be able to contribute to such a great project. :) -- sven fuchs svenfuchs@artweb-design.de artweb design http://www.artweb-design.de grünberger 65 + 49 (0) 30 - 47 98 69 96 (phone) d-10245 berlin + 49 (0) 171 - 35 20 38 4 (mobile)
on 05.12.2007 10:51
James, I've committed my work again on Monday. Like I've mentioned previously I probably won't have that much time to work on Engines until the weekend, probably even Wednesday next week. Besides the ActionMailer there are two things that I couldn't get working without further investigation: Arbitrary code mixing: the test on line 34 in http://dev.rails-engines.org/repository/file/test/engines/rails_2.0/test/unit/arbitrary_code_mixing_test.rb Model/Lib loading precedence: http://dev.rails-engines.org/repository/file/test/engines/rails_2.0/test/unit/model_and_lib_test.rb (ignore the note about functional/view_loading_test.rb in there) I've got the load_paths test working though: http://dev.rails-engines.org/repository/file/test/engines/rails_2.0/test/unit/load_path_test.rb Am 03.12.2007 um 01:15 schrieb Sven Fuchs: > Great. >> declaration of "controllers and helpers" out of the depths of the > >>> core code as > big win to be able to deactivate it by commenting out the line in >> The distinction here is that the only file in our "config" directory >> your work so far Sven! > d-10245 berlin + 49 (0) 171 - 35 20 38 4 (mobile) > > > > _______________________________________________ > Engine-Developers mailing list > Engine-Developers@lists.rails-engines.org > http://lists.rails-engines.org/listinfo.cgi/engine-developers-rails-engines.org -- sven fuchs svenfuchs@artweb-design.de artweb design http://www.artweb-design.de grünberger 65 + 49 (0) 30 - 47 98 69 96 (phone) d-10245 berlin + 49 (0) 171 - 35 20 38 4 (mobile)