Fix symlink regression #541

robwise · 2016-09-05T03:00:59Z

This change is

robwise · 2016-09-05T03:07:56Z

coveralls · 2016-09-05T03:13:26Z

Coverage decreased (-0.07%) to 82.534% when pulling 642ba75 on rob/fix-symlink-regression into ebf079b on master.

coveralls · 2016-09-05T03:15:12Z

Coverage decreased (-0.07%) to 82.534% when pulling 642ba75 on rob/fix-symlink-regression into ebf079b on master.

coveralls · 2016-09-05T03:37:26Z

Coverage decreased (-0.07%) to 82.534% when pulling a17f7fd on rob/fix-symlink-regression into ebf079b on master.

dzirtusss · 2016-09-05T10:14:04Z

Reviewed 3 of 3 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

lib/react_on_rails/assets_precompile.rb, line 40 [r2] (raw file):

      if file_or_symlink_exists_at_path?(symlink_path)
        puts "React On Rails: Removing existing invalid symlink or file #{symlink_path}"
        `rm -f "#{symlink_path}"`

I think we should not use any OS calls in gem at all. Why not to change to FileUtils or smth? Some previous errors were because somebody tried to use filenames with spaces and all that process get broken.

Comments from Reviewable

justin808 · 2016-09-05T10:30:12Z

Review status: all files reviewed at latest revision, 1 unresolved discussion.

lib/react_on_rails/assets_precompile.rb, line 40 [r2] (raw file):

Previously, dzirtusss (Sergey Tarasov) wrote…

I think we should not use any OS calls in gem at all. Why not to change to FileUtils or smth? Some previous errors were because somebody tried to use filenames with spaces and all that process get broken.

I'm OK with that change, @dzirtusss

Comments from Reviewable

When we changed this method in #513 we introduced a regression due to the difference between calling the shell's `ln -s` command and using Ruby's `File.symlink` command. Specifically, the former would not error if the symlink already existed, while the latter did throw an error. Because it is sometimes the case that the symlink will already exist, throwing in this case is not desirable. This should have not been a problem, however, as this scenario was supposed to have been properly handled, but that code was not correct. This commit fixes that code.

robwise · 2016-09-05T16:49:58Z

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

lib/react_on_rails/assets_precompile.rb, line 40 [r2] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I'm OK with that change, @dzirtusss

Done.

Comments from Reviewable

justin808 · 2016-09-06T02:34:15Z

Overall good. Couple questions. Plus, can we get a couple unit tests that show the changes?

Reviewed 2 of 3 files at r1, 1 of 4 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

lib/react_on_rails/assets_precompile.rb, line 31 [r3] (raw file):

      # File.exist?(symlink_path) will check the file the sym is pointing to is existing
      # File.lstat(symlink_path).symlink? confirms that this is a symlink
      valid_symlink_already_exists = File.exist?(symlink_path) && File.lstat(symlink_path).symlink?

Can this be a method and end with ?? and have a unit test?

lib/react_on_rails/assets_precompile.rb, line 85 [r3] (raw file):

          # We want the gz ones as well if they exist
          if File.exist?(@assets_path.join("#{rails_digested_filename}.gz"))

Why do we need to check this here?

lib/react_on_rails/assets_precompile.rb, line 123 [r3] (raw file):

    private

    def file_or_symlink_exists_at_path?(path)

unit test?

Comments from Reviewable

robwise · 2016-09-06T19:56:42Z

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

lib/react_on_rails/assets_precompile.rb, line 31 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Can this be a method and end with ?? and have a unit test?

I didn't write this, I just renamed the variable

lib/react_on_rails/assets_precompile.rb, line 85 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

Why do we need to check this here?

it'll throw if you try to symlink a file that doesn't exist, so all of the tests you wrote would fail without this

lib/react_on_rails/assets_precompile.rb, line 123 [r3] (raw file):

Previously, justin808 (Justin Gordon) wrote…

unit test?

you can't unit test private methods, not to mention I don't see the need to unit test whether core Ruby methods work

Comments from Reviewable

justin808 · 2016-09-07T06:26:39Z

@robwise or @dzirtusss Please confirm my last commit. I changed the things I commented on.

coveralls · 2016-09-07T06:37:23Z

Coverage increased (+0.3%) to 82.924% when pulling 168f3c8 on rob/fix-symlink-regression into ebf079b on master.

robwise · 2016-09-08T03:49:51Z

spec/react_on_rails/assets_precompile_spec.rb

@@ -62,6 +62,48 @@ module ReactOnRails
          end.to raise_exception(AssetsPrecompile::SymlinkTargetDoesNotExistException)
        end
      end
+
+      it "creates a proper symlink when a file exists at destination" do


you should specify which destination you are talking about

justin808 · 2016-09-08T06:21:31Z

Review status: 1 of 3 files reviewed at latest revision, 12 unresolved discussions.

spec/react_on_rails/assets_precompile_spec.rb, line 66 [r4] (raw file):

Previously, robwise (Rob Wise) wrote…

you should specify which destination you are talking about

destination is where you want to create the symlink

what's a better word for this?

Here's the docs:

symlink(old_name, new_name) → 0 click to toggle source
Creates a symbolic link called new_name for the existing file old_name. Raises a NotImplemented exception on platforms that do not support symbolic links.

suggestion?

spec/react_on_rails/assets_precompile_spec.rb, line 77 [r4] (raw file):

Previously, robwise (Rob Wise) wrote…

is it a valid symlink or invalid symlink? It doesn't do anything if it's valid, just outputs a message
context "when there is already a valid symlink in place" do

   it "outputs a message saying that it need not perform any action" do

@robwise you're thinking of the calling code that does this check. This method doesn't care.

spec/react_on_rails/assets_precompile_spec.rb, line 83 [r4] (raw file):

Previously, robwise (Rob Wise) wrote…

I don't get what's going on here compared to your test description

??

spec/react_on_rails/assets_precompile_spec.rb, line 89 [r4] (raw file):

Previously, robwise (Rob Wise) wrote…

Unnecessary, this is a TempFile

Not true in this case, as I created a symlink.

spec/react_on_rails/assets_precompile_spec.rb, line 105 [r4] (raw file):

Previously, robwise (Rob Wise) wrote…

why are you doing this, it's a TempFile?

Not true in this case, as I created a symlink.

Comments from Reviewable

coveralls · 2016-09-10T02:58:37Z

Coverage increased (+0.3%) to 82.924% when pulling d32a8cb on rob/fix-symlink-regression into ebf079b on master.

Allow filtering react_on_rails specs with :focus

1c4affb

robwise force-pushed the rob/fix-symlink-regression branch from 47fc608 to 642ba75 Compare September 5, 2016 03:03

robwise added the review-needed label Sep 5, 2016

robwise added 2 commits September 5, 2016 12:49

Fix possible open resource leak in AssetsPrecompile spec

7ecdc36

robwise force-pushed the rob/fix-symlink-regression branch from a17f7fd to 7ecdc36 Compare September 5, 2016 16:49

Add tests that confirm the new changes

168f3c8

robwise reviewed Sep 8, 2016
View reviewed changes

whitespace

d32a8cb

justin808 merged commit 211fed0 into master Sep 10, 2016

justin808 deleted the rob/fix-symlink-regression branch September 10, 2016 02:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symlink regression #541

Fix symlink regression #541

robwise commented Sep 5, 2016 •

edited by justin808

Loading

robwise commented Sep 5, 2016

coveralls commented Sep 5, 2016

coveralls commented Sep 5, 2016

coveralls commented Sep 5, 2016

dzirtusss commented Sep 5, 2016

justin808 commented Sep 5, 2016

robwise commented Sep 5, 2016

justin808 commented Sep 6, 2016

robwise commented Sep 6, 2016

justin808 commented Sep 7, 2016

coveralls commented Sep 7, 2016

robwise Sep 8, 2016 •

edited

Loading

justin808 commented Sep 8, 2016

coveralls commented Sep 10, 2016

Fix symlink regression #541

Fix symlink regression #541

Conversation

robwise commented Sep 5, 2016 • edited by justin808 Loading

robwise commented Sep 5, 2016

coveralls commented Sep 5, 2016

coveralls commented Sep 5, 2016

coveralls commented Sep 5, 2016

dzirtusss commented Sep 5, 2016

justin808 commented Sep 5, 2016

robwise commented Sep 5, 2016

justin808 commented Sep 6, 2016

robwise commented Sep 6, 2016

justin808 commented Sep 7, 2016

coveralls commented Sep 7, 2016

robwise Sep 8, 2016 • edited Loading

Choose a reason for hiding this comment

justin808 commented Sep 8, 2016

coveralls commented Sep 10, 2016

robwise commented Sep 5, 2016 •

edited by justin808

Loading

robwise Sep 8, 2016 •

edited

Loading