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

Remove Tempfile #6485

Merged
merged 1 commit into from Aug 31, 2018

Conversation

Projects
None yet
5 participants
@straight-shoota
Contributor

straight-shoota commented Aug 4, 2018

This removes Tempfile and places its methods in File and Dir as proposed in #5939 and standardizes the arguments as proposed in #5938. All tempfile methods now receive prefix, suffix (and optional dir) arguments. There are also overload for only one argument suffix (and optional dir) because it is pretty common to create a temp file with a specific extension.

Previously, require "tempfile" was needed in order to use this feature. Now that it is integrated into File I think it makes sense to have it in corelib.

I'm not sure if tempfile with block should perhaps actually delete the tempfile. Or if there could be some other mechanism to simplify automatic deletion of tempfiles. This could be discussed in a follow-up issue, though.
Same goes for the addition of a method to create a temporary directory (see #4096). The only issue with this is that Dir.tempdir would also be a fitting name for this but it is now used to query the tempdir environment setting.
Related to that, there could also be a future improvement to allow modifying the global tempdir, probably in a way similar to Time::Location.local.

Fixes #5939 and #5938

# It is the caller's responsibility to remove the file when no longer needed.
def self.tempfile(prefix : String?, suffix : String?, *, dir : String = Dir.tempdir, encoding = nil, invalid = nil)
fileno, path = Crystal::System::File.mktemp(prefix, suffix, dir)
new(path, fileno, blocking: true, encoding: encoding, invalid: invalid)

This comment has been minimized.

@asterite

asterite Aug 4, 2018

Contributor

Why blocking true?

This comment has been minimized.

@straight-shoota

straight-shoota Aug 4, 2018

Contributor

Not sure... I just copied it form Tempfile#initialize.

This comment has been minimized.

@RX14

RX14 Aug 7, 2018

Member

all files use blocking IO anyway because O_NONBLOCK has no effect on files.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-tempfile branch 3 times, most recently from 83d62c8 to e511c80 Aug 4, 2018

@ysbaddaden

This comment has been minimized.

Member

ysbaddaden commented Aug 5, 2018

As said in issues about this, I don't like this breaking change. I prefer to keep concerns distinct, and really see no need to push this into corelib.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-tempfile branch from e511c80 to e87ba4e Aug 5, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 5, 2018

I think we established that there is little reason for Tempfile being a separate type with exactly the same behaviour as File.

An alternative option might be to keep Tempfile as a module and have these methods defined there, but return an ordinary File instance. This would reduce the breaking changes, it would just be that Tempfile.new returns File instead of Tempfile (etc.).

But the changes in the method signatures need to be breaking anyways in order to provide a sane API.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-tempfile branch from e87ba4e to 089e9fe Aug 5, 2018

@RX14

RX14 approved these changes Aug 7, 2018

I like this change.

# It is the caller's responsibility to remove the file when no longer needed.
def self.tempfile(prefix : String?, suffix : String?, *, dir : String = Dir.tempdir, encoding = nil, invalid = nil)
fileno, path = Crystal::System::File.mktemp(prefix, suffix, dir)
new(path, fileno, blocking: true, encoding: encoding, invalid: invalid)

This comment has been minimized.

@RX14

RX14 Aug 7, 2018

Member

all files use blocking IO anyway because O_NONBLOCK has no effect on files.

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-tempfile branch from 089e9fe to cf9eb74 Aug 7, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 7, 2018

Rebased

@straight-shoota straight-shoota force-pushed the straight-shoota:jm/feature/remove-tempfile branch from cf9eb74 to 400f70f Aug 7, 2018

@sdogruyol

Thank you @straight-shoota 👍

@RX14 RX14 requested a review from bcardiff Aug 9, 2018

@straight-shoota

This comment has been minimized.

Contributor

straight-shoota commented Aug 22, 2018

@ysbaddaden Would you feel more comfortable if we put the tempfile methods back in Tempfile namespace (as module) and just return File?
This would separate tempfile methods from regular File methods. And it wouldn't need to be in core lib.

@RX14

This comment has been minimized.

Member

RX14 commented Aug 23, 2018

I see no reason why they can't be in the corelib, it's not particularly much more code (most of the code would be required as part of crystal/system anyway) and it's only dependency is libc. It doesn't drag in anything more to the corelib. This PR is fine as-is.

@RX14

RX14 approved these changes Aug 23, 2018

@asterite

I like this change. It removes one type from the standard library (Tempfile) which isn't much different than a file. So I find this way simpler than the previous one.

@j8r j8r referenced this pull request Aug 30, 2018

Merged

New feature: local cache #35

@RX14 RX14 merged commit 57f5473 into crystal-lang:master Aug 31, 2018

4 checks passed

ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RX14 RX14 added this to the 0.27.0 milestone Aug 31, 2018

@straight-shoota straight-shoota deleted the straight-shoota:jm/feature/remove-tempfile branch Sep 3, 2018

@RX14 RX14 referenced this pull request Sep 14, 2018

Open

Feature/mktmpdir #4096

ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018

@straight-shoota straight-shoota referenced this pull request Oct 25, 2018

Merged

Release 0.27.0 #6992

@007lva 007lva referenced this pull request Nov 3, 2018

Merged

Support Crystal 0.27.0 #1

omarroth added a commit to omarroth/crystal that referenced this pull request Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment