Fixed bug to allow filenames with spaces in assets_precompile.rb #513
Conversation
|
@dzirtusss There's no need for a separate PR. You can do a |
|
It was my mistake in that I asked Sergey to rename his fixes to a branch. The original review comments are in #510. I should have asked Sergey to rebase to origin/master on the remote, which means creating a named remote, such as "shakacode" (rather than origin, as that's Sergey's fork) and rebasing on top of that. My bad. |
|
Reviewed 3 of 3 files at r1. CHANGELOG.md, line 10 [r1] (raw file):
Update this to 513. I'll do this. And we should have the author! lib/react_on_rails/assets_precompile.rb, line 46 [r1] (raw file):
👍 spec/react_on_rails/assets_precompile_spec.rb, line 29 [r1] (raw file):
It's worth noting that digest_filename and filename are all relative paths in the manifest. I would have probably kept in the removed test that verified that confirms the value of readLink is not the absolute path of filename. Then again, it's clear from the code a few lines above that calls File.basename. 😄 spec/react_on_rails/assets_precompile_spec.rb, line 40 [r1] (raw file):
Critical that we fixed this. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. CHANGELOG.md, line 10 [r1] (raw file):
|
|
ok Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. Comments from Reviewable |
|
Reviewed 1 of 1 files at r2. CHANGELOG.md, line 10 [r2] (raw file):
see other names in the changelog for the format Comments from Reviewable |
|
couple comments Review status: all files reviewed at latest revision, 3 unresolved discussions. spec/react_on_rails/assets_precompile_spec.rb, line 29 [r1] (raw file):
|
|
Changes Unknown when pulling 799b60a on dzirtusss:fix-spaces into * on shakacode:master*. |
|
Done Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions. CHANGELOG.md, line 10 [r2] (raw file):
|
|
one more comment Reviewed 2 of 2 files at r3. CHANGELOG.md, line 10 [r3] (raw file):
almost...see other entries. We use the github id. Comments from Reviewable |
|
Be sure to run Offenses: Tasks: TOP => default => lint => lint:lint => lint:rubocop (See full trace by running task with --trace) Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. Comments from Reviewable |
|
Done Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion. CHANGELOG.md, line 10 [r3] (raw file):
|
|
Changes Unknown when pulling 1bead68 on dzirtusss:fix-spaces into * on shakacode:master*. |
|
Reviewed 2 of 2 files at r4. Comments from Reviewable |
|
Done, squashed commits Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending. Comments from Reviewable |
|
Changes Unknown when pulling 035ecb0 on dzirtusss:fix-spaces into * on shakacode:master*. |
|
Review status: Comments from Reviewable |
all files reviewed at latest revision, all discussions resolved, all commit checks successful.
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.
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.
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.
Fix AssetsPrecompile#symlink_file logic 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. * Allow filtering react_on_rails specs with :focus * Fix possible open resource leak in AssetsPrecompile spec
#510 moved to a branch
This change is