Comments for lazyatom's engines  

Azimux – would you mind re-finding what broke?

The reason I added this was because the patch came with a test that indeed failed, and I saw the absolute path directory being created. So, I was happy to apply the fix, given it passed the tests.

In general, I’m not hugely happy with the implementation of this whole chunk of the plugin. I would welcome a cleaner, clearer rewrite of this that we can merge back in.

The current ‘master’ version only works with 2.1.1 or greater – once some of the fixes for Rails 2.2 (see http://engines.lighthouseapp.com/projects/10178/tickets/39-rails-22rc1-actionviewtemplatefinder for example) are working, I’ll update this.

For Rails 2.2.0, the last condition fails!

Is it intentional that this version requires Rails 2.2.1?

I also didn’t see the point of this code. Originally I fixed this by removing that code, and this worked with no problems at all in all of my projects. I was going to submit this as the patch at first, but when I wrote the test for it, it failed some other test. I put it back in and changed it to the patch I submitted and then all the tests passed. I didn’t feel like figuring out why that other test was failing at the time. I probably should have recorded which one it was. But you can probably figure it out by deleting the code and running the tests.

Okay, I see – thanks for explaining that. I’ve never actually seen the case when there are files in the root directory, however it might exist.

I think the only situation when this code is necessary is when you have an “assets” dir with files but no subdirectories (source_dirs is empty). In this case, the first block will take care of creating the destination dir.

I suppose the block could be replaced by:

FileUtils.mkdir_p(destination)

Anyway, I haven’t tested it.

I have to ask the question, what is the point of this code anyway? I couldn’t understand why it is there. The directory is created anyway, further down in that function. Are you able to fill me in on what this is about?

http://github.com/snowblink/engines/commit/a3108e7277c8dd64336308f7c444d1f1ca547bc6

Rake 0.8.1 resolve_args returns [task_name, arg_name_list, prerequisites]. Does line 13 need to be amended?

Ok, you’re right, I were wrong. I still had this block from the Engines list in my development.rb which caused the difference:

config.after_initialize { Dependencies.load_once_paths = [] }

This causes Engines to be reloaded.

I still don’t get the rational behind that change though. :)

Sure – I guess the tricky thing is that here we want pop the top path (the app), push a new path on (the next plugin) and then push the app path back on. I’m not sure if it’s a common usecase.

It could be that the ordering the engines plugin uses for priority just isn’t useful. As it stands at the moment, the earlier a plugin is loaded, the lower its path appears in the list of load paths, view paths and so on. It would almost certainly be easier to implement if that was flipped around…

config.after_initialize shouldn’t be fired after every request – only at the end of initialization… hmm. Can you confirm that this block is called after every request?

Hmm, this change actually seems to do the opposite of what the comment says. I get a “stack level too deep” error because Engines::RailsExtensions::AssetHelpers is included at server startup and subsequent requests (not the first request though). Thus, the asset helper methods get alias_chained again every time and an endless loop results.

When I revert that change and have Engines.init called only on plugin loading stage that behaviour vanishes.

The plugin’s init.rb file gets eval’d only once anyways, so I don’t get the rational behind this change at all … maybe that was one beer to much though ;)

We should probably talk about this, part of the motivation for tidying up the internals of AV was to make this kinda thing easier, not harder ;)