crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.5k stars 1.62k forks source link

Add a method to create and clean up a temporary directory to the standard library #14244

Open apainintheneck opened 10 months ago

apainintheneck commented 10 months ago

Discussion

What aspect of the language would you like to see improved?

A method should be added to the standard library which allows users to create and clean up a temporary directory.

It should have the ability to not only create a temporary directory but also expose a temporary directory in a block and then clean up that directory when the block scope ends.

What are the reasons?

There have been attempts in the past to add a method that creates and cleans up temporary directories. This shows that users of other programming languages expect to find this functionality in the standard library.

Previous PRs:

I was personally looking to diff two directories and being able to create temporary directories in that scenario would be useful.

Building a naive solution for this is not difficult (see https://github.com/crystal-lang/crystal/pull/4096#issuecomment-753679477) but it'd be great to have a more robust solution that users can use without having to worry about the implementation.

Optionally add one (or more) proposals to improve the current situation.

  1. Add Dir.mktempdir method and recursive option to Dir.delete

As far as I can tell, the main reason that the other PRs and discussions stalled out was that there was disagreement over how tightly coupled the Dir, File and FileUtils modules should be. Specifically there were concerns raised about using FileUtils in either Dir or File.

One of the proposals that got buried in the discussion was to add new functionality to the Dir module so that it wouldn't have to rely on FileUtils anymore to remove a temporary directory at the end of a block. Adding a new Dir.delete(directory, recursive = false) method would help solve these problems because we would be able to avoid using FileUtils.rm_r.

I believe that the change to Dir.delete change was first proposed by @ysbaddaden in https://github.com/crystal-lang/crystal/issues/5231#issuecomment-343418239.

Proposed Changes:

Potential Follow-ups:


  1. Add FileUtils.mktemp_d

The advantage here is that we don't have to worry about this method sharing code with FileUtils though it does mean it's not exactly where people would expect it to be. It also acknowledges that this command does multiple things including deleting files which might not be what you expect to find in the Dir module.

It seems like there is disagreement about whether adding new methods to FileUtils is even a good idea in the first place so this might be controversial.

Proposed Changes:

Related Links

straight-shoota commented 10 months ago

Sounds good. I think it makes a lot of sense to have a Dir method for recursive deletion.

icefoxen commented 2 months ago

Nitpick for the future: on Unix systems this should call the system's mkdtemp(3) rather than making up its own names like the current File.tempfile does, if possible. (There's equivalents on Windows but idk what they're called.) Otherwise you can have race condition attacks where something nefarious makes the directory for you and then has access to the stuff you put in it.