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

Behaviour for joining multiple S3Paths? #75

Open
nickrobinson251 opened this issue Mar 26, 2020 · 3 comments
Open

Behaviour for joining multiple S3Paths? #75

nickrobinson251 opened this issue Mar 26, 2020 · 3 comments

Comments

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Mar 26, 2020

I've no idea what behaviour is expected or intended for joining together S3Paths

There is no test case for it. But here's what happens right now. We should maybe consider if this is what is wanted, then either add a test or fix it.

julia> abc = S3Path("s3://a/b/c/");  # trailing slash on `c`

julia> xyz = S3Path("s3://x/y/z");

julia> abc / xyz
p"s3://a/b/c/y/z/"  # trailing slash
julia> abc = S3Path("s3://a/b/c");  # no trailing slash on `c`

julia> abc / xyz  
p"s3://a/b/c/y/z"  # no trailing slash
julia> a = S3Path("s3://a");

julia> x = S3Path("s3://x/");

julia> a / x
p"s3://a"
@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Mar 26, 2020

Also, we special case joining strings:

julia> @which join(abc, "foo/bar")
join(prefix::S3Path, pieces::AbstractString...) in AWSS3 at /Users/nick/.julia/packages/AWSS3/UmYRI/src/s3path.jl:94

But for Paths in general, we fall back to the join defined on AbstractPaths in FilePathsBase.

julia> @which join(abc, p"foo/bar")
join(prefix::T, pieces::Union{AbstractString, AbstractPath}...) where T<:AbstractPath in FilePathsBase at /Users/nick/.julia/packages/FilePathsBase/Oyg1p/src/path.jl:230

This gives correct for WindowsPaths and PosixPaths

julia> abc / WindowsPath("foo/bar")
p"s3://a/b/c/foo/bar"

julia> abc / PosixPath("foo/bar")
p"s3://a/b/c/foo/bar"

But if we decide this behaviour in the opening post is not what we want for joining multiple S3Paths, we'll want to add a new join method.

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Apr 1, 2020

i think joining multiple S3Paths is probably just nonsense.

If there's a public API to get the segments (is there?), then you can just explicitly do

julia> abc = S3Path("s3://a/b/c/");

julia> xyz = S3Path("s3://x/y/z");

julia> join(abc, xyz.segments...)
p"s3://a/b/c/y/z"

So i think join(::S3Path, ::S3Path) should be made a MethodError. Although if this isn't considered a bug-fix, then it is a breaking change, since it makes a non-error an error.

@nickrobinson251
Copy link
Contributor Author

actually, afaict FilePathsBase does not test joining Paths to Paths at all https://github.com/rofinn/FilePathsBase.jl/blob/820903b2c8521f6a871ba7651763bcbce6754087/src/test.jl#L330

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant