Fixed bug to allow filenames with spaces in assets_precompile.rb

[dzirtusss]

Fixed bug to allow filenames with spaces in assets_precompile.rb its test spec, closes #505

---

This change is

[coveralls]

Coverage increased (+0.02%) to 82.316% when pulling 6291e54 on dzirtusss:master into 63436ab on shakacode:master.

[justin808]

@dzirtusss Tip: Feel free to review your own PR first putting in questions and comments. These are useful to the reviewer and it's a place to put discussions/comments that don't deserve to be in code comments.

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

lib/react_on_rails/assets_precompile.rb, line 44 [r1] (raw file):

        puts "React On Rails: Symlinking \"#{target}\" to \"#{symlink}\""
        dest_path = File.join(@assets_path, target_sub_path)
        FileUtils.chdir(dest_path)

we should probably use the block syntax so that the chdir only applies to the code inside of the block.

---

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

        dest_path = File.join(@assets_path, target_sub_path)
        FileUtils.chdir(dest_path)
        File.symlink(File.expand_path(target_filename), File.expand_path(symlink_filename))

be careful with the file expand

we need a test to ensure that we're never using absolute paths

I suspect that you don't need the expand_path.

please be sure to verify this in the tests

---

spec/react_on_rails/assets_precompile_spec.rb, line 28 [r1] (raw file):

                                                               assets_path.join("test images")))
        digest_filename = "#{filename} digest"
        AssetsPrecompile.new(assets_path: assets_path)

if you ever check a symlink into github, then you can see the difference between the symlink that is the minimum relative path versus a full path.

---

spec/react_on_rails/assets_precompile_spec.rb, line 33 [r1] (raw file):

        expect(assets_path.join(digest_filename).lstat.symlink?).to be true
        expect(File.identical?(assets_path.join(filename),
                               assets_path.join(digest_filename))).to be true

Ideally, I'd like to see new tests with a file name that does not have spaces, so we can run the tests without the fix and see just these tests fail.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.02%) to 82.316% when pulling d03cb71 on dzirtusss:master into c77d3b6 on shakacode:master.

[justin808]

Thanks @dzirtusss

---

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

be careful with the file expand

we need a test to ensure that we're never using absolute paths

I suspect that you don't need the expand_path.

please be sure to verify this in the tests

@dzirtusss Please mark addressed issues as "done" in reviewable.io. Thanks.

---

spec/react_on_rails/assets_precompile_spec.rb, line 31 [r2] (raw file):

        expect(File.readlink(assets_path.join(digest_filename)).to_s)
          .not_to eq(File.expand_path(assets_path.join(filename)).to_s)
      end

CC: @alleycat-at-git Check this out.

---

Comments from Reviewable

[justin808]

@dzirtusss Please see https://github.com/shakacode/react_on_rails/blob/master/CONTRIBUTING.md.

We need to update the CHANGELOG.md.

Please add that entry and rebase on top of master.

[dzirtusss]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

lib/react_on_rails/assets_precompile.rb, line 44 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

we should probably use the block syntax so that the chdir only applies to the code inside of the block.

Agree 200%. chdir in block really interesting and easy construct, saves efforts and get clean code

---

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

be careful with the file expand

we need a test to ensure that we're never using absolute paths

I suspect that you don't need the expand_path.

please be sure to verify this in the tests

Agree. I don't need expand_path - added it to get global symlink. Now updated to local. Check for local/global path also added to test.

---

spec/react_on_rails/assets_precompile_spec.rb, line 28 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

if you ever check a symlink into github, then you can see the difference between the symlink that is the minimum relative path versus a full path.

Agree only relative paths for future

---

spec/react_on_rails/assets_precompile_spec.rb, line 33 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Ideally, I'd like to see new tests with a file name that does not have spaces, so we can run the tests without the fix and see just these tests fail.

I've splitted tests for 1) space 2) global links as a separate its. Than tested each against its revision. This is what you mean?

---

Comments from Reviewable

[dzirtusss]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

lib/react_on_rails/assets_precompile.rb, line 45 [r1] (raw file):

Previously, dzirtusss (Sergey Tarasov) wrote…

Agree. I don't need expand_path - added it to get global symlink. Now updated to local. Check for local/global path also added to test.

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.02%) to 82.36% when pulling 357a36d on dzirtusss:master into a22abeb on shakacode:master.

[dzirtusss]

Done

---

Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.02%) to 82.36% when pulling 1a4eb4a on dzirtusss:master into a22abeb on shakacode:master.

[coveralls]

Coverage increased (+0.02%) to 82.36% when pulling 1a4eb4a on dzirtusss:master into a22abeb on shakacode:master.

[dzirtusss]

Merge commits

---

Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[justin808]

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

spec/react_on_rails/assets_precompile_spec.rb, line 23 [r3] (raw file):

      end

      it "doesn't create global symlink" do

"creates a relative symlink, rather than an absolute path symlink"

---

spec/react_on_rails/assets_precompile_spec.rb, line 30 [r3] (raw file):

        expect(File.readlink(assets_path.join(digest_filename)).to_s)
          .not_to eq(File.expand_path(assets_path.join(filename)).to_s)

Expect the readlink to equal the digest_filename?

If we test for equality, that's a bit more of a sure thing.

Only having an "expectation not" can result in a test that passes due to an error in writing the test.

---

Comments from Reviewable

[justin808]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

spec/react_on_rails/assets_precompile_spec.rb, line 30 [r3] (raw file):

Expect the readlink to equal the digest_filename?

Expect the readlink to equal the filename?

---

Comments from Reviewable

[dzirtusss]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

spec/react_on_rails/assets_precompile_spec.rb, line 23 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

"creates a relative symlink, rather than an absolute path symlink"

Done.

---

spec/react_on_rails/assets_precompile_spec.rb, line 30 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Expect the readlink to equal the digest_filename?

Expect the readlink to equal the filename?

Done.

---

Comments from Reviewable

[justin808]

@dzirtusss Please try to reopen this one.