readdir must return strings for FilePathsBase compatibility

[ExpandingMan]

Currently doing readdir(p, join=true) returns an array of paths. This is a problem because FilePathsBase expects it to be strings in all cases.

This is breaking things pretty badly (e.g. even rm on directories is currently broken), so I'm hoping we can get this merged relatively quickly.

[ExpandingMan]

This is a little worrying because we are also testing against what is apparently the wrong behavior... I don't think FilePathsBase has changed, but I'm not entirely sure.

bors try

[ericphanson]

I don't think FilePathsBase has changed, but I'm not entirely sure

Yep, FilePathsBase made a patch release that added join=true and also broke AWSS3: #226 (comment) (see also #227).

We can make this change here*, but it would definitely need to be a breaking change for AWSS3.jl.

*: I think these are the wrong semantics, because it means read(readdir(...;join=true)[1]) now throws, but it would at least be consistent with FilePathsBase. Not really sure if it's worth it or not. As expressed in #226, I think we should stop trying to fulfill the FilePathsBase interface altogether, which would also definitely be a breaking change.

[ericphanson]

Needs breaking version bump if we are going to do this

[ExpandingMan]

In my opinion breaking with FilePathsBase would drastically diminish the usefulness of this package.

The current state of the package is rather broken, so this needs to be done until and unless you won't to re-implement everything.

bors try

[bors]

try

Already running a review

[ExpandingMan]

I'm not sure what's happening with bors. I thought it got stalled out because I made another commit, but now it says it's already running and doesn't want to run.

[ericphanson]

I'm not sure what's happening with bors. I thought it got stalled out because I made another commit, but now it says it's already running and doesn't want to run.

I think Bors might be broken; last time I tried it in #229 it timed out. Not sure what's going on though, maybe @mattBrzezinski can look into it?

The current state of the package is rather broken, so this needs to be done until and unless you won't to re-implement everything.

Well, we could just restrict compat to non-broken FilePathsBase? This PR could do that instead. Ref #226 (comment)

Or we could just add the workaround

Base.parse(::Type{P}, path::P; kwargs...) where {P<:S3Path} = path

which is probably the nicest fix for now-- shouldn't break any existing code, and will stop the errors from #227

[ExpandingMan]

Restricting the FilePathsBase versoin without fixing this seems like a bad idea, because then this package will just get stuck on an effectively unmaintained version of it. If AWSS3 drops FilePathsBase, then so be it, but before that has happened it doesn't make sense to restrict it.

I also don't think adding a method to Base.parse makes much sense, the FilePathsBase behavior is logical: its return type does not depend on join.

So yes, even though I disagree that it is desirable, there is a case to be made for dropping FilePathsBase, but in the meantime, it seems better to try to keep things working properly.

[ericphanson]

So yes, even though I disagree that it is desirable, there is a case to be made for dropping FilePathsBase, but in the meantime, it seems better to try to keep things working properly.

Ok, but breaking the fact that you can read the outputs readdir(...;join=true) is not keeping things working properly! That's like a pretty key bit of using S3Paths in generic code. So if we really want to do that for the sake of consistency, this needs to be a breaking release. Or we can add the no-op parse method to workaround the breakage from https://github.com/rofinn/FilePathsBase.jl/pull/152/files#diff-4e2b33a7304f53abf36fe9499936056b993077836e4b72a5c5ae016d8c6d6d1fR713.

[ExpandingMan]

I'm re-thinking my stance against extending Base.parse. I still think it is ultimately a bad idea, but I think we are headed to some kind of reckoning, so maybe it makes sense to merely patch things up before that happens.

Btw, please let me know what you think about my draft PR to FilePathsBase in that thread.

[bors]

try

Timed out.

[omus]

Test formatting changes

[ExpandingMan]

Sorry, was busy. Let's try running again

bors try

[bors]

try

Build succeeded:

• Julia 1.3 - ubuntu-latest - x64
• Julia 1 - ubuntu-latest - x64
• Julia 1.3 - macOS-latest - x64