FileUtils: Add `ln`, `ln_s`, and `ln_sf` #5421

woodruffw · 2017-12-20T21:49:17Z

These new methods in the FileUtils module are based largely on
their Ruby equivalents.

The major difference between this and the Ruby implementation is that
ln and ln_s raise ArgumentErrors in Crystal on errors,
while Ruby raises Errno. Other than that, the only differences
are the lack of an options argument (consistent with other
Crystal FileUtils methods).

I didn't get any comments on the RFC I made (#5369), so I don't know if these changes are welcome or not. Please let me know if they aren't!

Papierkorb · 2017-12-20T22:01:53Z

src/file_utils.cr

+  # FileUtils.ln_s("/var/log", "logs")
+  # FileUtils.ln_s("src", "/tmp")
+  # ```
+  def ln_s(old_path : String, new_path : String)


You're not renaming, so old and new aren't really precise. Something like source and destination would be clearer.

Papierkorb · 2017-12-20T22:12:00Z

Hi,

I was confused at first, but I saw that your code is more than plain aliases. Which is good.

I know that Ruby does #ln_s and friends, but I never liked that scheme. #ln should be #link or #hardlink, and #ln_s something like #symlink or #symbolic_link. Much clearer in naming to the uninitated, and easier to search for.

Second, instead of the f variants, have a force: argument instead. This makes it much more powerful to use, easier to read, and less to remember for the developer. You can force the developer to use a named argument using an empty * argument:

def symlink(source, destination, *, force = false); ...; end

# Usage would look like this:
symlink("foo", "bar", force: user_config.force?) # No need for a clutter-if with an argument
symlink("foo", "bar", force: true) # Passing a constant value is also easy
symlink("foo", "bar", true) # Less clear: What's "true" for? What is true in this case?

I'm no core maintainer, so someone else has more say in this. I think the shown functionality in this PR (With some cleaning) is useful and should be part of Crystal.

Not part of this PR, but IMHO, we should rethink the FileUtils module however. Maybe we can get rid of it by adding this functionality into their sibling methods in File and Dir?

Papierkorb · 2017-12-20T22:13:14Z

src/file_utils.cr

+  # ```
+  # FileUtils.ln(["vim", "emacs", "nano"], "/usr/bin")
+  # ```
+  def ln(old_paths : Enumerable(String), dest_dir : String)


I really like how you accept a Enumerable(String) there 👍

Papierkorb · 2017-12-20T22:14:11Z

src/file_utils.cr

+  # ```
+  # FileUtils.ln_sf("foo.c", "bar.c")
+  # ```
+  def ln_sf(old_path : String, new_path : String)


As noted in the comment, use a force argument instead of having a separate forcing version.

Papierkorb · 2017-12-20T22:15:33Z

spec/std/file_utils_spec.cr

+
+  describe "ln" do
+    it "should create a hardlink" do
+      path1 = "/tmp/crystal_ln_test_#{Process.pid}"


Use Tempfile.dirname to get the path to the temporary directory instead of hardcoding it. Consider using Tempfile itself to generate temporary "test" files instead.

Do you think it would be a good idea to do this in another PR? /tmp is hardcoded all over this file, and might be good to update the tests all at once.

Yes in that case it would be a good follow-up PR.

Papierkorb · 2017-12-20T22:17:04Z

spec/std/file_utils_spec.cr

+    end
+
+    it "should create multiple hardlinks inside a destination dir" do
+      paths = 3.times.map { |i| "/tmp/crystal_ln_test_#{Process.pid + i}" }.to_a


Instead of x.times.map{ y }.to_a use Array.new(x){ y }

Papierkorb · 2017-12-20T22:18:05Z

spec/std/file_utils_spec.cr

+      path1 = "/tmp/crystal_ln_test_#{Process.pid}"
+      path2 = "/tmp/crystal_ln_test_#{Process.pid + 1}"
+
+      expect_raises Errno do


Would be nice if it could also check for the expected errno value itself.

Does this look reasonable?

expect_raises Errno do FileUtils.ln(path1, path2) end Errno.value.should eq(Errno::ENOENT)

I wasn't sure if Errno.value could be clobbered by other threads.

Doesn't the errno exception contain the ENOENT as part of its message? If so, you can use expect_raises(Errno, /ENOENT/) to expect the method to raise an Errno, with a message containing the regex.

However, errno is a Thread Local variable, so your code should work too

It looks like ENOENT gets stringified to "No such file or directory":

Expected Errno with message matching /ENOENT/, got #<Errno: Error creating link from /tmp/crystal_ln_test_28472 to /tmp/crystal_ln_test_28473: No such file or directory> with backtrace:

I guess I could match on that, but it feels a little less reliable. I'll go with the Errno.value check (I don't know how I forgot that errno is thread local 🙂).

Better to use exception object for that:

ex = expect_raises Errno do FileUtils.ln(path1, path2) end ex.errno.should eq(Errno::ENOENT)

Thanks @Sija!

Papierkorb · 2017-12-20T22:18:37Z

spec/std/file_utils_spec.cr

+        FileUtils.ln_sf(path1, path2)
+        File.symlink?(path1).should be_false
+        File.symlink?(path2).should be_true
+      ensure


Empty ensure block

Thanks! Filled it in 😄

Papierkorb · 2017-12-20T22:27:11Z

src/file_utils.cr

+    end
+  end
+
+  # Like `#ln_s(String, String)`, but overwrites `new_path` if it exists and is not a directory.


Decent documentation overall 👍 Could you add a short description to each usage sample on what it'll do? That improves skim-ability of the docs:

# Create a symbolic link, pointing from bar to foo. FileUtils.ln_sf("foo", "bar")

woodruffw · 2017-12-20T22:38:03Z

Thanks for the review and comments @Papierkorb!

I know that Ruby does #ln_s and friends, but I never liked that scheme. #ln should be #link or #hardlink, and #ln_s something like #symlink or #symbolic_link. Much clearer in naming to the uninitated, and easier to search for.

I agree 100%, but I wasn't sure whether that would be appropriate given the scheme already in use in FileUtils -- cp_r, mkdir_p, and rm_rf all use the "unix-command-plus-flags" naming scheme already, so I didn't know if it would be appropriate to deviate from it. I'll let one of the core maintainers decide what the approach should be here 😄

Not part of this PR, but IMHO, we should rethink the FileUtils module however. Maybe we can get rid of it by adding this functionality into their sibling methods in File and Dir?

I'm of two minds on that. Merging FileUtils into File and Dir would avoid a lot of confusion, but many of the FileUtils versions also add Unix semantics (like cp and ln copying into a destination directory implicitly). That additional behavior really comes in handy when writing quick scripts, but would probably be confusing if added to the equivalent methods in File and Dir.

RX14 · 2017-12-21T11:42:55Z

@Papierkorb The purpose of FileUtils is to keep the same naming as the commandline. I dislike the module, and I think that it should be a shard personally. In addition, more work needs to be done on base File/Dir and possibly add Path. That being said, all of that is far outside the scope of this PR and we should keep the conventions of FileUtils in this PR.

woodruffw · 2017-12-29T18:09:08Z

Anything further I should do on this PR?

RX14 · 2017-12-29T18:20:00Z

src/file_utils.cr

+  # # Create a symbolic link pointing from bar.c to foo.c, even if bar.c already exists
+  # FileUtils.ln_sf("foo.c", "bar.c")
+  # ```
+  def ln_sf(src_path : String, dest_path : String)


There should be a version of this which takes multiple paths.

I can add this, but it'll functionally be a duplicate of ln_s (since taking multiple paths only makes sense if dest is a directory, and the force behavior is only applied if dest is a file). Should I still do it?

Why is force only applied if dest is a file? There can still be name clashes if the destination is a directory.

RX14 · 2017-12-29T18:20:23Z

src/file_utils.cr

+  # FileUtils.ln_sf("foo.c", "bar.c")
+  # ```
+  def ln_sf(src_path : String, dest_path : String)
+    File.delete(dest_path) if File.file?(dest_path)


If dest_path is a dir, this won't work.

That's consistent with ln -sf's behavior -- if dest_path is a dir, then dest_path/src_path is created rather than dest_path being deleted.

Yes but dest_path/src_path can exist, -f imples that file is deleted, which doesn't happen here.

RX14 · 2017-12-31T12:03:59Z

src/file_utils.cr

+    if File.file?(dest_path)
+      File.delete(dest_path)
+    elsif File.directory?(dest_path)
+      dest_file = File.join(dest_path, File.basename(src_path))


At this point we just worked out the dest_path, why not call File.symlink directly from this function in both cases?

woodruffw · 2018-04-11T01:07:29Z

Anything else I can do here? 😄

RX14 · 2018-04-11T22:10:29Z

src/file_utils.cr

+    if File.file?(dest_path)
+      File.delete(dest_path)
+      File.symlink(src_path, dest_path)
+    elsif File.directory?(dest_path)


What if dest_path is neither a file or directory, for example if it's nonexistant? It should still link.

if File.directory?(dest_path) dest_path = File.join(dest_path, File.basename(src_path)) end File.delete(path) if File.file?(dest_path) File.symlink(src_path, dest_path)

Have you added specs for that? Also please please push update commits instead of squashing. It makes it far easier tor view just the changes on a large PR.

I'll add a spec in a bit (and sorry for the squashes).

These new methods in the FileUtils module are based largely on their Ruby equivalents. The major difference between this and the Ruby implementation is that `ln` and `ln_s` raise `ArgumentError`s in Crystal on errors, while Ruby raises `Errno`. Other than that, the only differences are the lack of an `options` argument (consistent with other Crystal `FileUtils` methods).

Sija · 2018-05-11T15:49:03Z

Seems like GTG, merge 🕦 ?

oprypin · 2018-05-14T17:24:06Z

Did we seriously just merge a method named ln_sf?

woodruffw · 2018-05-14T17:46:32Z

Did we seriously just merge a method named ln_sf?

I still think this is the right naming, both for consistency with Ruby's FileUtils and in line with the general distinction between File, Dir, and FileUtils -- File and Dir represent the general notions of files and directories, and FileUtils represents the UNIX-y and userspace-y convenience layer that's frequently laid over them.

RX14 · 2018-05-14T17:46:59Z

@oprypin we already have a method for rm_rf. It's not nice but it's how the module works.

I just want the module out of the stdlib entirely but unfortunately thats not really possible until we get Path done.

* FileUtils: Add `ln`, `ln_s`, and `ln_sf` These new methods in the FileUtils module are based largely on their Ruby equivalents. The major difference between this and the Ruby implementation is that `ln` and `ln_s` raise `ArgumentError`s in Crystal on errors, while Ruby raises `Errno`. Other than that, the only differences are the lack of an `options` argument (consistent with other Crystal `FileUtils` methods). * spec: Add FileUtils.ln_sf test for nonexistent dest

Papierkorb reviewed Dec 20, 2017

View reviewed changes

woodruffw force-pushed the fileutils-ln branch 3 times, most recently from 02bcb51 to 01ca324 Compare December 20, 2017 23:17

RX14 requested changes Dec 29, 2017

View reviewed changes

woodruffw force-pushed the fileutils-ln branch 2 times, most recently from 8f1c4ad to 6974e92 Compare December 31, 2017 04:53

RX14 requested changes Dec 31, 2017

View reviewed changes

woodruffw force-pushed the fileutils-ln branch from 6974e92 to 96fde85 Compare December 31, 2017 17:44

RX14 requested changes Apr 11, 2018

View reviewed changes

woodruffw force-pushed the fileutils-ln branch from 96fde85 to 8f7b642 Compare April 12, 2018 00:23

woodruffw force-pushed the fileutils-ln branch from 8f7b642 to 84c02fe Compare April 12, 2018 00:24

spec: Add FileUtils.ln_sf test for nonexistent dest

f6b59c7

RX14 approved these changes Apr 12, 2018

View reviewed changes

sdogruyol approved these changes Apr 12, 2018

View reviewed changes

RX14 merged commit 07c7066 into crystal-lang:master May 14, 2018

RX14 added this to the Next milestone May 14, 2018

RX14 added kind:feature topic:stdlib labels May 14, 2018

woodruffw deleted the fileutils-ln branch May 14, 2018 16:24

FileUtils: Add ln, ln_s, and ln_sf #5421

FileUtils: Add ln, ln_s, and ln_sf #5421

Conversation

woodruffw commented Dec 20, 2017

Choose a reason for hiding this comment

Papierkorb commented Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Dec 20, 2017

RX14 commented Dec 21, 2017

woodruffw commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija commented May 11, 2018

oprypin commented May 14, 2018

woodruffw commented May 14, 2018

RX14 commented May 14, 2018 • edited

FileUtils: Add `ln`, `ln_s`, and `ln_sf` #5421

FileUtils: Add `ln`, `ln_s`, and `ln_sf` #5421

Papierkorb commented Dec 20, 2017 •

edited

woodruffw Dec 20, 2017 •

edited

Sija Dec 20, 2017 •

edited

RX14 Apr 12, 2018 •

edited

RX14 commented May 14, 2018 •

edited