Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Tempfile.tempname accepting block #5937

Conversation

straight-shoota
Copy link
Member

This PR adds an overload to Tempfile.tempname which yields the path and ensures it is cleaned up after exiting the block.

A possible use case is detailed in #5811

Followup on #5360

@straight-shoota
Copy link
Member Author

straight-shoota commented Apr 10, 2018

#4096 is similar but explicitly for directories and creates a directory at the path.

@woodruffw
Copy link
Contributor

woodruffw commented Apr 11, 2018

Awesome! I should have thought about this when I initially added tempname 🙂

@ysbaddaden
Copy link
Contributor

That's weird: I'm asking for a String but suddenly the methods deletes a file or directory on disk? Requiring tempfile would now inject `FileUtils, too.

It's a good snippet to put into test helpers, but I'm not sure this should be pushed to stdlib...

@Sija
Copy link
Contributor

Sija commented Apr 11, 2018

I second @ysbaddaden here, concept itself might be useful enough to add it as a new method, or feature behind an argument (sth like Tempfile.new(cleanup: true)), though mixing it with #tempname which is supposed to return just that - name, String - seems confusing at least.

@straight-shoota
Copy link
Member Author

So you'r basically objecting the name, not the concept? I see the point of confusion, but on the other hand it just follows a common idiom in Crystal, to have a yielding method overload which adds some cleanup to the non-yielding behaviour.

@ysbaddaden Removing a directory recursively should either be implemented outside FileUtils or we need to accept it being included in some other places. It's just such a common feature. We've already had this issue in other places.

@RX14
Copy link
Contributor

RX14 commented Apr 11, 2018

The core problem is that we're yielding a String, not a Path object. If you consider a hypothetical future where we merge the Path object, and this PR yielded a path object and then called #delete on the path after the block ran, I don't think there would be any complaint.

The core problem with this PR is that it has to work within the confines of Crystal's existing conflation of paths and strings, which is why I'm ok with this PR. We'll fix up this PR when we merge #5550.

This PR also shows exactly why File vs FileUtils vs Dir is stupid (we don't implement recursive directory deletion in the corelib, because it's arbitrarily defined on FileUtils instead of File, and now we need it in the corelib). Another thing which will be fixed by #5550. This PR is essentially before it's time, and makes perfect sense if #5550 is merged.

@Sija
Copy link
Contributor

Sija commented Apr 13, 2018

I object. Such behaviour would be more fitting for a Tempfile#open_and_cleanup(&block) method, not one returning temporary String. Mixing generation of random String with deleting such file on disk would be just simply confusing.

@ysbaddaden
Copy link
Contributor

This pull request isn't ahead of its time, it should just be a method in spec. If it happens to be very useful across many projects, then maybe we could consider moving this to stdlib, otherwise let's not bother. With a proper stdlib it's just a few lines to write anyway.

Let's fix things in the right order, instead of adding new features on a whim. Recursive deletion is indeed important: let's work on that! Forget about Tempfile and FileUtils and work on Path, rework Dir and File, so they become so good that maybe we'll just drop Tempfile and FileUtils in the end.

@straight-shoota
Copy link
Member Author

I'm closing this. Since Tempfile was removed in #6485 this PR would need a refactoring. I'm not going to invest that without core members supporting this.

#5951 solved the most important use case in specs, but I still think this would also be useful as a method outside of specs. Maybe there will be a new PR at some point...

@straight-shoota straight-shoota deleted the jm/feature/tempfile-yield branch September 4, 2018 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants