Skip to content
This repository has been archived by the owner on Aug 29, 2019. It is now read-only.

Commit

Permalink
Browse files Browse the repository at this point in the history
Added a fix for absolute paths being created into plugin_assets direc…
…tory. [#6 state:resolved], [#26 state:resolved] - apologies for the delay, and thanks to Azimux for the patch.
  • Loading branch information
lazyatom committed Oct 24, 2008
1 parent 5cbfd7d commit 031d8c1
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 1 deletion.
2 changes: 1 addition & 1 deletion lib/engines.rb
Expand Up @@ -144,7 +144,7 @@ def mirror_files_from(source, destination)
source_files -= source_dirs

unless source_files.empty?
base_target_dir = File.join(destination, File.dirname(source_files.first))
base_target_dir = File.join(destination, File.dirname(source_files.first).gsub(source, ''))
FileUtils.mkdir_p(base_target_dir)
end

Expand Down
4 changes: 4 additions & 0 deletions test/unit/assets_test.rb
Expand Up @@ -24,6 +24,10 @@ def test_public_files_have_been_copied_from_test_assets_plugin
assert File.exist?(File.join(Engines.public_directory, 'test_assets', 'subfolder', 'file_in_subfolder.txt'))
end

def test_engines_has_not_created_duplicated_file_structure
assert !File.exists?(File.join(Engines.public_directory, "test_assets", RAILS_ROOT))
end

def test_public_files_have_been_copied_from_test_assets_with_assets_dir_plugin
Engines::Assets.mirror_files_for Engines.plugins[:test_assets_with_assets_directory]

Expand Down

5 comments on commit 031d8c1

@ioquatix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Knack
Copy link

@Knack Knack commented on 031d8c1 Oct 28, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ioquatix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@azimux
Copy link

@azimux azimux commented on 031d8c1 Oct 29, 2008

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lazyatom
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Please sign in to comment.