Feature/mktmpdir by bramswenson · Pull Request #4096 · crystal-lang/crystal

bramswenson · 2017-03-02T04:53:22Z

This is simply a rebase of #2911 to current master (03-01-2017)

Dir class assumes the responsability of resolving the system tmpdir from the Tempfile class. Reference: - http://rxr.whitequark.org/mri/source/lib/tmpdir.rb

The mkdtemp function creates a directory with a unique name. If it succeeds, it overwrites template with the name of the directory, and returns template. As with mktemp and mkstemp, template should be a string ending with ‘XXXXXX’. The directory is created using mode 0700 References: - http://www.gnu.org/software/libc/manual/html_node/Temporary-Files.html - http://ruby-doc.org/stdlib-2.0.0/libdoc/tmpdir/rdoc/Dir.html

As discussed on #2911

marceloboeira · 2017-03-02T09:13:20Z

👍

RX14 · 2017-03-02T09:29:42Z

+  end
+
+  # Creates a new temporary directory within the lifecycle
+  # of the given block and destroys it, and its content, later on.


...and its content, when the block returns

RX14 · 2017-03-02T09:33:32Z

+  # Dir.tmpdir # => "/tmp"
+  # ```
+  def self.tmpdir
+    unless tmpdir = ENV["TMPDIR"]?


This is a bit confusing, how about

tmpdir = ENV["TMPDIR"]? || "/tmp"

marceloboeira · 2017-03-02T09:39:59Z

@bramswenson do you want to make the changes? because if I would require access to your fork/branch.

bramswenson · 2017-03-02T14:32:43Z

@marceloboeira @RX14 Done! Thanks!

marceloboeira · 2017-03-02T14:35:54Z

@bramswenson great!

asterite · 2017-03-03T12:20:42Z

+  # Dir.rm_r("dir")
+  # Dir.rm_r("file.cr")
+  # ```
+  def self.rm_r(path : String)


This belongs to FileUtils, not Dir

Dir can rm_r , FileUtils can rm_r too .
just like String can have add method , Number can add too .
Right ?

@sevk stop trolling please.

asterite · 2017-03-03T12:24:42Z

@bramswenson Thank you for this!

In general the PR looks very good. However:

I'm not sure about adding Dir.rm_r. In general we have simple methods in File and Dir, and more complex methods in FileUtils. FileUtils has methods that you can find in bash, like rm_r or mkdir_p.
I'm also not sure about Dir.tmpdir, but I think it's OK (similar to Ruby, related to Dir and not Tempfile), but this is a small breaking change and we should note it.

I'm all the time unsure about File, Dir, FileUtils and where to put things. Maybe things are too complex/messy right now. I'll discuss this with the team, maybe we can just put all methods in File and Dir and remove FileUtils.

asterite · 2017-03-03T12:25:56Z

For now we can merge your change if it doesn't introduce breaking changes (Use Tmpfile.dirname instead of Dir.tmpdir and keeps FileUtils.rm_r instead of Dir.rm_r)). That's at least if we want this included in the next patch release.

bramswenson · 2017-03-03T18:31:15Z

@asterite Thanks for the kind words, but I did nothing but the rebase to master out of selfish desire to use this feature ;) @marceloboeira Did the actual work and there was reasonable discussion about the move between Dir and FileUtils in #2911 between @marceloboeira and @RX14. I really don't have a dog in that fight and will be happy to see the feature wherever you all decide is most appropriate. Thanks!

bramswenson · 2017-03-03T18:33:58Z

I will say that the removal of FileUtils in preference for File and Dir makes sense to me. I also think these methods should closely resemble their unix counterparts in naming as unix file modification executable names are common knowledge (or should be). But that is just my 2 cents.

marceloboeira · 2017-03-06T07:43:35Z

I didn't have time to take a look at the comments yet folks, but I will do it soon. @bramswenson thanks for the invitation to your repository and the efforts on making it happen. 👍

bew · 2017-09-01T21:31:56Z

I was about to (suggest to) add this!
@bramswenson, will you work on this anytime soon ? (I'd really like something like that to refactor some compiler specs)

About naming I was thinking, since we already have the Tempfile class, we could have Tempdir ?
And put the two in a Temporary module with a simple API like:

Temporary::File.new # gives a file
Temporary::Dir.new # gives a dir

Temporary::Dir.open do |dir|
  # do something with the `dir`
  puts dir.path
end # the block ensures that the temp dir is deleted

Temporary.create_name("foo.XXX") # gives a (file/dir)name following a pattern

# etc..

RX14 · 2017-09-01T23:26:47Z

I think we shouldn't have "tempdir" and "tempfile" classes, just methods on Dir and File which create tempoary versions of themselves. Why does Tempfile currently not inherit File? It makes little sense to me.

bew · 2017-09-01T23:33:41Z

Why does Tempfile currently not inherit File? It makes little sense to me.

I thought the same.

marceloboeira · 2018-09-14T08:18:01Z

@RX14 any updates here?

RX14 · 2018-09-14T12:49:42Z

@marceloboeira Tempfile was removed in #6485. We still need to decide what to do about FileUtils, I say that most of it's methods belong on Path once that is merged.

j8r · 2021-01-03T21:35:44Z

Too old, I think should be closed now. A new PR can be opened if wanted.

As now, to create a temp dir:

dir = File.tempname 
Dir.mkdir dir
begin
  # do things
ensure
  FileUtils.rm_rf dir
end

marceloboeira added 4 commits March 1, 2017 20:43

Move system tmpdir logic to Dir class

6cd19f6

Dir class assumes the responsability of resolving the system tmpdir from the Tempfile class. Reference: - http://rxr.whitequark.org/mri/source/lib/tmpdir.rb

Move rm_r to Dir

717b0f9

As discussed on #2911

Remove FileUtils from Dir

30b55d0

bramswenson mentioned this pull request Mar 2, 2017

Add temporary directory creation on Dir class #2911

Closed

RX14 requested changes Mar 2, 2017

View reviewed changes

bramswenson added 2 commits March 2, 2017 06:30

Update Dir.tmpdir documentation per @RX14 suggestions

989fb15

Update tmpdir test for env var per @RX14 suggestions

7b8e7f6

asterite reviewed Mar 3, 2017

View reviewed changes

bcardiff added kind:feature pr:wip topic:stdlib labels Mar 20, 2017

bew mentioned this pull request Sep 1, 2017

Start Refactor tool init specs #4915

Merged

straight-shoota mentioned this pull request Nov 17, 2017

FileUtils: refactoring #5231

Open

This was referenced Apr 10, 2018

Add Tempfile.tempname accepting block #5937

Closed

Improve interface of Tempfile methods #5938

Closed

Remove class Tempfile #5939

Closed

straight-shoota mentioned this pull request Aug 4, 2018

Remove Tempfile #6485

Merged

straight-shoota closed this Jan 4, 2021

apainintheneck mentioned this pull request Jan 16, 2024

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

Open

Uh oh!

Conversation

bramswenson commented Mar 2, 2017

Uh oh!

marceloboeira commented Mar 2, 2017

Uh oh!

RX14 Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

marceloboeira Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

RX14 Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

marceloboeira Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

marceloboeira commented Mar 2, 2017

Uh oh!

bramswenson commented Mar 2, 2017

Uh oh!

marceloboeira commented Mar 2, 2017

Uh oh!

asterite Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

sevk Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

Sija Mar 11, 2017

Choose a reason for hiding this comment

Uh oh!

asterite commented Mar 3, 2017

Uh oh!

asterite commented Mar 3, 2017

Uh oh!

bramswenson commented Mar 3, 2017

Uh oh!

bramswenson commented Mar 3, 2017

Uh oh!

marceloboeira commented Mar 6, 2017

Uh oh!

bew commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RX14 commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bew commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marceloboeira commented Sep 14, 2018

Uh oh!

RX14 commented Sep 14, 2018

Uh oh!

j8r commented Jan 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

bew commented Sep 1, 2017 •

edited

Loading

RX14 commented Sep 1, 2017 •

edited

Loading

bew commented Sep 1, 2017 •

edited

Loading