Revise overloaded temp methods #150
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.
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.
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.
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.
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.
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.
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.
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.
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
tempdirto be define for all path types.Closes #141