-
Notifications
You must be signed in to change notification settings - Fork 17
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
Revise overloaded temp methods #150
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #150 +/- ##
==========================================
- Coverage 91.84% 91.68% -0.17%
==========================================
Files 12 12
Lines 1190 1202 +12
==========================================
+ Hits 1093 1102 +9
- Misses 97 100 +3
Continue to review full report at Codecov.
|
function Base.download(url::AbstractString, localfile::P) where P <: AbstractPath | ||
mktemp(P) do fp, io | ||
function Base.download(url::AbstractString, localfile::AbstractPath) | ||
mktmp() do fp, io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mktemp(P)
previously returned a SystemPath
by default. We've changed this, so we've made this explicit to avoid infinite recursion on the download
call below.
|
||
Base.mktempdir(fn::Function, P::Type{<:AbstractPath}) = mktempdir(fn, tempdir(P)) | ||
mktmpdir(fn::Function) = mktempdir(fn, SystemPath) | ||
Base.tempname(P::Type{<:AbstractPath}; kwargs...) = tempname(tempdir(P); kwargs...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behaviour for tempname
now calls tempdir(P)
, so we require that all new path types define that method for their specific type.
Base.mktempdir(P::Type{<:AbstractPath}; kwargs...) = mktempdir(tempdir(P); kwargs...) | ||
Base.mktempdir(fn::Function, P::Type{<:AbstractPath}; kwargs...) = mktempdir(fn, tempdir(P); kwargs...) | ||
|
||
function Base.tempname(parent::AbstractPath; prefix="jl_", cleanup=true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefix
was also added in 1.2 (though not as consistently).
|
||
function Base.tempname(parent::AbstractPath; prefix="jl_", cleanup=true) | ||
isdir(parent) || throw(ArgumentError("$(repr(parent)) is not a directory")) | ||
fp = parent / string(prefix, uuid1()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to uuid1
over uuid4
. For older releases of Julia, these function depended on the GLOBAL_RNG
and could in theory result in collisions if folks were resetting while generating temp files and directories.
function temp_cleanup(fp::AbstractPath) | ||
atexit() do | ||
# Might not work in all cases, but default to recursively deleting the path on exit | ||
rm(fp; force=true, recursive=true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the specific suggestion made in #141. It might not always work, but it seems better than nothing?
@@ -113,8 +113,8 @@ function isexecutable(fp::SystemPath) | |||
return ( | |||
isexecutable(s.mode, :ALL) || | |||
isexecutable(s.mode, :OTHER) || | |||
( usr.uid == s.user.uid && isexecutable(s.mode, :USER) ) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My editor fixed some formatting issues from now till my next commend.
function Base.mktemp(parent::T) where T<:SystemPath | ||
fp, io = mktemp(string(parent)) | ||
|
||
Base.tempdir(::Type{<:SystemPath}) = Path(tempdir()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the SystemPath
specific defaults to the system.jl file.
@@ -70,5 +70,6 @@ Base.read(fp::TestPath) = read(test2posix(fp)) | |||
Base.write(fp::TestPath, x) = write(test2posix(fp), x) | |||
Base.chown(fp::TestPath, args...; kwargs...) = chown(test2posix(fp), args...; kwargs...) | |||
Base.chmod(fp::TestPath, args...; kwargs...) = chmod(test2posix(fp), args...; kwargs...) | |||
Base.tempdir(::Type{TestPath}) = posix2test(tempdir(PosixPath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of needing to add a Base.tempdir
overload.
To support new 1.3+ kwargs (e.g., prefix, cleanup).
NOTE: This should be a breaking release as we now require
tempdir
to be define for all path types.Closes #141