Cleanup File & FileUtils

[bcardiff]

• Add missing methods to File of features available in FileUtils: same_content?/cmp copy/cp
• Only FileUtils.cp treats the dest as a potential directory. File.copy works with file paths.
• I left a TODO related to Socket#sendfile #8926, copy_file_range is a Linux-only optimization for FD to FD copying #8919 for File.copy optimizations via syscall.
• Deprecate Dir.rmdir in favor of Dir.delete (File.delete is the file counter-part)
• Another breacking change is the straight removal of FileUtils.cmp(IO, IO) (which seems totally misplaced)
• Allow File.same? and File.touch with Path arguments.

Related to #5231.

The APIs that are not strict aliases in FileUtils are

• FileUtils.touch, .mv, .mkdir, .mkdir_p, .rm: because they allow multiple files
• FileUtils.cp: because it allows destination to be a directory (and allows multiple files)
• FileUtils.cp_r: there is no Dir counter-part and it handle the semantic of File vs Dir source / destination.
• FileUtils.ln[_s|_sf]: because it allows destination to be a directory and allows multiple files
• FileUtils.rm_r[f]: there is no Dir counter-part and allows multiple directories.

So, the following are the criteria that limited the scope of the PR are:

• I don't think we need to allow multiple input in File/Dir API.
• I'm not sure about doing the special treatment in the destination path that cp_r, ln_s will fit in File API.
• There is also some treatment of File.symlink? in rm_r that I hesistated to move into Dir, and so, I didn't implement Dir.delete(recursive: true) proposal of FileUtils: refactoring #5231 (comment)

[bcardiff]

Finally I forgot to say that I would rather leave FileUtils, specially if the semantics are not 100% translatable to Dir/File since they will offer some terminal semantics I found very handy.

Side notes:

• In C# File.copy does not accept a directory as target ref
• In Ruby File.copy was deprecated in favor of FileUtils ref 🙃

[j8r]

I think FileUtils aliases have to be phased out.

The module is about File system related utilities, and it provides useful ones. It was obviously very inspired by Ruby.

However, providing UNIX-friendly aliases bring little. A library could provide them, a la Shell JS. This way, more "missing" aliases could be provided.

Last but not least, encouraging people using the other similar methods avoid them to require "file_utils" unnecessarily.

To sumup, benefits:

• more consistent style
• less modules required
• less UNIXisms (or CMDisms) (good, just my opnion)

[j8r]

This method can be deprecated too. I don't know how the core members sees the future of this module, but we can start now removing unnecessary aliases at least.

[bcardiff]

I think is valid to have this unix-isms in a module. And FileUtils offers some operations that I think does not fit neatly in Dir or File.

If you say this can be deprecated because is a plain alias, then I don't agree. My intention is to keep FileUtils with similar methods and semantics as you would find in a unix shell.

I think @maiha has some opinions regarding why unix-isms are nice.

[j8r]

I was only pointing duplicated methods. There are lots of terminal methods missing, which are very useful and very commonly used like ls, head, tail, wc, find, etc...

I don't think they will be accepted (will they?), because all aliases at the end can be better written in more idiomatic Crystal.

[j8r]

Anyway, sorry it starts to be out of scope 😅 .
Let's keep it then.

[maiha]

This is not a "plain alias" as @bcardiff says, nor is it a "duplicate method".

Since Crystal is an "object-oriented" language, the core classes such as File and Dir, their APIs are provided in an object-oriented paradigm.

On the other hand, since UNIX is a "procedural" concept, it has different APIs.

Here, the two paradigm API entry points are each defined and their implementations are only shared.

So, if you want to clean up, I think it's a good idea to remove the confusing annotation part of "NOTE: Alias of".

[j8r]

Both are called in a functional way – they are class methods, not instance methods. Only the name change.

I think FileUtils is just there because of Ruby, a reason why I believe this is nobody asked to add ls, which is one of the most common and useful command.

Utils is also quite bad, if the main purpose of the module is to provide a command-like interface, it would better be called FileCommandsor Shell.

[j8r]

I don't say everything on FileUtils has to go to trash. I'm only saying we don't have to copy blindly 1-1 the Ruby API, and review the actual module to see what is often used (cp, dir methods) and those not or confusing like cmp or pwd (I searched both on GitHub before saying this)

[maiha]

nobody asked to add ls

Well, why not try to think of FileUtils as File (mutable) Utils?

[asterite]

Members of the core team like FileUtils so it's going to stay. I know opinions are diverse here but the decision is already taken, so please let's not discuss removing anything.

[j8r]

I didn't know cmp was a command. Same here, I also think it can be deprecated.

[bcardiff]

I get that you want to deprecate the whole FileUtils module. But why you said this method can be deprecated specifically?

[j8r]

cmp is not very common, and quite cryptic name. Not sure this alias really helps.

[bcardiff]

Rebased on master. And addressed @RX14 feedback.

I changed what happens on File.copy when destination exists to match the Ruby and cp default semantics. #9175 (comment)

[bcardiff]

@oprypin the permissions support in windows is not there yet, right? The new specs that asserts the permissions are set on the destination file in File.copy are failing.

[oprypin]

I don't know, wasn't working on that

[bcardiff]

Ok, thanks.

Ignoring those specs for now. Otherwise the CI complains with:

Failures:

  1) File .copy copies permissions
     Failure/Error: File.info(out_path).permissions.should eq(File::Permissions.new(0o700))

       Expected: rwx------ (0o700)
            got: rw-rw-rw- (0o666)

     # \spec\std\file_spec.cr:1260

  2) File .copy overwrites existing destination and permissions
     Failure/Error: File.info(out_path).permissions.should eq(File::Permissions.new(0o700))

       Expected: rwx------ (0o700)
            got: rw-rw-rw- (0o666)

     # \spec\std\file_spec.cr:1275

[oprypin]

Oh. fileutil is not tested on Windows, so makes sense that stuff is broken if you're moving it out

crystal/spec/win32_std_spec.cr

Line 62 in 2cbe65b

# require "./std/file_utils_spec.cr"

[Sija]

@bcardiff Would be sensible to close #9170 perhaps, along with rest of the fixes while you're at it :)

[bcardiff]

Done @Sija although I would say it is unrelated to the goal of this PR.

I checked Ruby code and it covers some scenarios we don't. For example, it deals with symlinks in the destination path by removing them.