Create directories when using Legolas.write(::PosixPath, ...)
#26
Conversation
| @@ -140,8 +140,7 @@ end | |||
| # | |||
| # TODO: upstream improvements to Arrow.jl to obviate these? | |||
|
|
|||
| write_full_path(path::AbstractString, bytes) = (mkpath(dirname(path)); Base.write(path, bytes)) | |||
| write_full_path(path, bytes) = Base.write(path, bytes) | |||
| write_full_path(path, bytes) = (mkpath(dirname(path)); Base.write(path, bytes)) | |||
There was a problem hiding this comment.
this is one of those things that's annoying because some path types benefit from it (e.g. local file paths where directories are actually a real/tangible thing) where others don't (S3 "paths" don't actually involve directories, just prefixes with a directory-like naming convention)
so will this cause annoying behaviors for path types like S3Paths (e.g. trigger the creation of a dummy prefix before creating the actual object for no good reason)
There was a problem hiding this comment.
I was under the impression mkpath for S3Path was a no-op. The current behaviour seems rather silly so I'll go address that too
There was a problem hiding this comment.
Do you mean getting rid of https://github.com/JuliaCloud/AWSS3.jl/blob/47ba4534d23aebc19e7fe0eede471f3ff4eb256d/src/s3path.jl#L390-L399 ?
I feel like the actual thing we want here is a version of Base.write that will create any necessary parent directories when writing the file, as needed by the path type at hand
dealing with mkpath at seems like a red herring since this code really only cares about writing files/objects, and doesn't inherently care about directories at all
There was a problem hiding this comment.
the actual thing we want here is a version of Base.write that will create any necessary parent directories when writing the file, as needed by the path type at hand
I wonder if we could try to add such a function an interface function to FilePathsBase, define it for the path types we care about, and then just get rid of Legolas.write_full_path entirely in favor of that function (and introduce a slight dependency on FilePathsBase - "your path doesn't need to be an AbstractPath but does at least need to support this function")
There was a problem hiding this comment.
Actually, you know what - maybe we should just deprecate this function entirely. Why is Legolas in the business of creating intermediate paths for users at all? why not follow Base's lead and puts the onus on the caller to use mkpath beforehand if they need it?
Legolas' behavior is of course convenient when it's what you want, but maybe it really is generally more user-friendly (and at least more common) to error for local filesystem paths if they don't have the directories already made (and this shouldn't matter for S3Paths anyway)
There was a problem hiding this comment.
Actually, you know what - maybe we should just deprecate this function entirely. Why is Legolas in the business of creating intermediate paths for users at all? why not follow Base's lead and puts the onus on the caller to use
mkpathbeforehand if they need it?
I can get behind this.
There was a problem hiding this comment.
After looking a bit into the S3Path behaviour I think there is some room for improvement but I'm doubtful we can go the no-op route. I've made an issue outlining the various options for further discussion there: JuliaCloud/AWSS3.jl#224
|
closing in favor of #26 (comment)
I'll file a follow-up issue |
When using a
PosixPathfrom FilePathsBase.jl we fail to create the intermediate directories like we do forStringpaths. This should allow us to support both properly.TODO:
S3Pathmanually