Crystal: Undefined method 'split' for Path

[sudo-nice]

This code fails with Crystal 0.28.0 [639e4765f] (2019-04-17)

path = Path.new "/tmp/foo/bar"
Dir.mkdir_p path

Error in r.cr:2: instantiating 'Dir.class#mkdir_p(Path)'

Dir.mkdir_p path
^~~~~~~

in /usr/share/crystal/src/dir.cr:241: instantiating 'mkdir_p(Path, Int32)'

def self.mkdir_p(path, mode = 0o777)
  ^

  in /usr/share/crystal/src/dir.cr:244: undefined method 'split' for Path

components = path.split(File::SEPARATOR)

[asterite]

Dir isn't aware of Path yet. It seems only File is.

I wonder how can this be solved... imagine I want to define an API (a shard) that works with paths. Should I handle String | Path? Anyone will have to handle both of these things all the time? Maybe it's fine...

[j8r]

... Why using Dir in the first place? Keep using the Path object will be real nice, with methods like path.create_dir(all: true), with all to create all subdirectories.

[asterite]

@j8r I mean, it's probably a good idea, but the comment is confusing. It makes it look like path.create_dir is already a thing but it's not. So the only option for OP is to do Dir.mkdir_p(path.to_s)

[j8r]

What I've understood is the issue is mainly of creating a directory from a Path. I think @sudo-nice knows that Dir.mkdir_p works with String (thus with to_s), else he won't bother to do a Path.new from a String before.
I should be more explicit on the comment. about the proposed draft.

[bew]

Should I handle String | Path ?

@asterite I think creating a Path should be almost transparent (understand: very lightweight), so that all methods that deals with file paths can only take a Path as input. And when you have a String you simply create a Path.new some_str and give it.

So instead of:

input = "foo/bar"
Dir.mkdir_p input

You'd do:

input = "foo/bar"
Dir.mkdir_p Path.new(input)

This way Dir can be properly typed (and can't throw unhelpful error messages like OP' one), and only has to deal with Path.

It's also better semantic-wise, when you expect a path, you get a Path

[ysbaddaden]

@bew this is exactly what I don't want, and why I don't like Path.

To create a directory you must Dir.create(Path.new("tmp")) or Path.new("tmp").create_dir which are the opposite of developer friendliness when compared to Dir.create("tmp").

@asterite an alternative is to go with duck typing and have methods taking a path be type-less and have methods explicitly call .to_s.

[j8r]

There is also FileUtils#mkdir and FileUtils#mkdir_p.

We can have:

• Dir.create, which calls Crystal::System::Dir.create(path, mode)
• Dir.delete, which calls Crystal::System::Dir.delete(path)

Then Path can have dir wrappers around them:

• Path#create_dir(recursive: false)
• Path#remove_dir(recursive: false) (or delete_dir)

Other Dir creation/deletion methods in Dir and FileUtils can be deprecated.

This discussion starts to relate to https://github.com/crystal-lang/crystal/issues/5231

[bew]

@j8r please no, File & Dir interacts with the OS, Path only represents a file path (with extra method to get information on that path, but without interactions with the OS).

Adding OS methods to Path will only make things more complex imo...

@ysbaddaden Re-reading the initial issue about adding Path, I'm ok with accepting Path | String.
The Path object would be "just" a path on steroid where you can get extra information on it, etcc.. But it shouldn't _replace_ a path as a String

[j8r]

@bew Quite true, and Dir.create and FileUtils refactoring are related but another topics.

I think Path#to_s is fine. It doesn't seems to worth it to accept an Union of String | Path, I don't see a strong benefit. Most languages only accepts a String too.

One simple solution: Add String type restriction to each path argument of Dir and File class methods.

[sudo-nice]

I strongly believe that since Crystal has Path, it's plain wrong to keep treating String as Path as well. It's disturbing, trying to sit on two chairs.

[nickbclifford]

I completely agree with @sudo-nice. If there's going to be String options for these File/Dir operations anyway, why even have the Path type in the first place? Since the core team already went to all the trouble of implementing the type, it seems to me like a perfectly suitable _and_ more semantically-appropriate replacement for String in this case.

[konovod]

Both Path#to_s and Path.new approaches looks pretty wrong to me. I think Path | String is a resonable restriction - you either construct a Path and pass it, or hardcode a string.
Also it is easy to implement for a shard (or stdlib) - just add arg = Path.new(arg) unless arg.is_a? Path inside all methods that work with paths.

[j8r]

@konovod you mean

if arg.is_a? String
  arg = Path.new arg
end

This seems wrong, too.
Adding this piece of code everywhere will over complicate things already simple.
Dir and File are lower level than Path, it would be preferable that they don't depend on this wrapper.
Path knows/calls => Dir/File knows/calls => Crystal::System.
(Path already use Dir.current)

I don't know Java, apparently the Path object has a toFile() method that returns a File object representing the path.
Then, one can delete(), mkdir(), createNewFile().
In Crystal, this can't really be done as-is because, File is an IO.

[sudo-nice]

wrapper

What about making Path into first-class object with heavy surgery:

• add String#to_path method
• merge all File, Dir, FileUtils functionality as Path methods
• deprecate File, Dir, FileUtils

[asterite]

Let's make Dir also work with Path instances. Dir isn't that big and it's not a big change. Then String and Path will work seamlessly.

I understand that having String and Path might be confusing at first. But having both alternatives is really good:

• if you need a quick call without constructing a Path, or with a hardcoded value, just pass a String
• if your path was constructed via Path and its methods, just pass the resulting Path

That's the idea of having these methods accept String and Path, at least in my mind.

[RX14]

The options as far as I see them are retaining File and Dir methods (FileUtils needs to be gone) and making them accept String | Dir, or moving filesystem-interaction methods to Path. I'm in favor of moving filesystem mutation to Path.

Encouraging the current status quo of paths being a string is dangerous for the Windows port. Paths are very special strings which behave differently on different platforms and generally have a bunch of rules and reasons why they should not be treated as strings. Treating them as strings works fine on UNIX, where the rules are simple, but unfortunately you have to give up some minor amount of elegance for portability.

If we add Path{"foo", "bar"} == Path.new("foo/bar") or similar, then File.<method>("foo/bar") is the same length as Path{"foo/bar"}.<method>. And we can get rid of the somewhat arbitrary split between File and Dir (chmod works on all paths, not just files, yet it's on File!), and make Dir purely a dirfd(3) wrapper. And it's typed.

Edit: Path["foo"] exists already, so we don't even need to change anything.

[j8r]

Another thing (that I had suggested), this bring possibilities like Path.new("/aaa/bbb/ccc").each_parent &.create_dir to create subdirectories recursively instead of Dir.mkdir_p.

[j8r]

I would like to add, after coding on an heavy Filesystem project, that's super annoying to add everywhere to_s when dealing with path for symlinking/copying/real_path directories and files. Switching between File, Dir, Path and FileUtils, that supports sometimes String | Path or only String, is quite unpractical. I end up using Path#to_s everywhere.
It would be so much easier to have a single unified API - just saying this with my personal experience.
Java has either Path or File, and that's quite nice.