add `join` keyword to `readdir` #129

ericphanson · 2021-01-11T15:20:40Z

This keyword was added to Base in JuliaLang/julia#33113, which made it into Julia 1.4.

I couldn't run the tests locally, but I tested with an example and it seemed to work. The new tests in verify_files(path::AbstractPath) need the Base method, so will fail on Julia 1 - 1.3. Should I put them behind a VERSION check, or just drop them?

mattBrzezinski · 2021-01-12T04:41:51Z

bors try

bors · 2021-01-12T04:44:56Z

try

Build failed:

Julia 1.3 - ubuntu-latest - x64

mattBrzezinski · 2021-01-12T04:47:48Z

I couldn't run the tests locally, but I tested with an example and it seemed to work.

Any particular errors which popped up? I assume that it's probably a credential thing. At the very least you need credentials in ENV VARs / ~/.aws/config / ~/.aws/credentials files with full S3 perms.

The new tests in verify_files(path::AbstractPath) need the Base method, so will fail on Julia 1 - 1.3. Should I put them behind a VERSION check, or just drop them?

I think keeping this behind a VERSION check would be best. Keeping packages on the same LTS as JuliaLang is the most easy way of handling things. Once 1.6 comes out as LTS, we can then dump the version check and set a new min ver.

ericphanson · 2021-01-12T12:38:48Z

Any particular errors which popped up? I assume that it's probably a credential thing. At the very least you need credentials in ENV VARs / ~/.aws/config / ~/.aws/credentials files with full S3 perms.

Ah, this would be it, I'm on a restricted role. I could set something up but for this PR hopefully CI will be enough. Would you mind re-running bors? I think I fixed the issues in my last two commits.

mattBrzezinski · 2021-01-12T15:37:48Z

bors try

bors · 2021-01-12T15:40:36Z

try

Build failed:

Julia 1.3 - ubuntu-latest - x64

ericphanson · 2021-01-12T17:12:42Z

sorry, looks like I had a copy-paste error there, try again?

mattBrzezinski · 2021-01-12T17:23:39Z

bors try

bors · 2021-01-12T17:27:39Z

try

Build succeeded:

ericphanson · 2021-01-14T13:15:23Z

Bump / Is anything I need to do here? 🙂

mattBrzezinski · 2021-01-14T17:04:54Z

@rofinn has more strong opinions on S3Path stuff than I do.
I think this is fine, maybe locking the join=true as a separate function behind a VERSION check would be my only comment.

rofinn

Seems fine to me. Just a couple minor suggestions, I've also made an issue to add this to FilePathsBase.jl as well.

src/s3path.jl

rofinn · 2021-01-14T20:33:32Z

@mattBrzezinski Do we need to manually trigger bors or something?

mattBrzezinski · 2021-01-14T20:35:20Z

@mattBrzezinski Do the tests not run anymore for this repo? I know Sam O'Connor ran the live tests in his personal account, but I thought we changed that when we started using bors?

Yes they work w/ bors using the Invenia Public CI account. I assume you're referring to the original body. Eric had responded in this thread;

Ah, this would be it, I'm on a restricted role. I could set something up but for this PR hopefully CI will be enough.

ericphanson · 2021-01-15T10:28:10Z

thanks for the review! I've made those changes.

mattBrzezinski · 2021-01-15T18:05:22Z

bors r+

bors · 2021-01-15T18:09:27Z

Build succeeded:

rofinn · 2021-11-18T16:55:55Z

src/s3path.jl

+        sort && sort!(results)
+
+        # Return results, possibly joined with the root path if join=true
+        return join ? joinpath.(fp, results) : results


Hmm, one thing I just realized about this is that it introduces a type instability cause join determines whether we return strings or paths.

Yeah, it does-- I've been using this and for me having the functionality has been well worth it, but it might depend on your use-case. Generally either I am using it in the REPL (in which case it doesn't matter), or in code that generally ends up like

for file in readdir(path; join=true) # some kind of filtering... endswith(string(file), ".arrow") || continue process_file!(results, file) end

and the function-barrier (process_file!) means the instability is not an issue.

I'm trying to remember, is there a reason you didn't want to use readpath? I'm wondering if for now I should just add those flags to both readpath and readdir, but just make the only difference that one return paths and one returns strings? In the future, I may make a breaking release which unifies the API and always returns paths for readdir.

Ah, is that what readpath does? I didn't know that function exists and it's not really documented what it does:

julia> using FilePathsBase help?> readpath search: readpath readpath(fp::P) where {P <: AbstractPath} -> Vector{P}

Usually I stick to the Base API since I might have a mix of S3Paths and Base (string) paths.

(To be clear, join doesn't just return S3Path's vs strings, but rather it determines or not it's the full path, including the bucket etc, or just the filename).

(To be clear, join doesn't just return S3Path's vs strings, but rather it determines or not it's the full path, including the bucket etc, or just the filename).

Yeah, I was just wondering if it would have made more sense to do joinpath.(string(fp), results) in the readdir function and left returning paths to readpath?

I'd rather accept the type instability than to try to explain to users (and remember myself) why it gives back a full path but you can't actually use it to read bytes etc

Okay. I've opted away from the type instability in FilePathsBase since I have less control over how it'll be used. It also means I can support that keyword without introducing a breaking change. I may change readdir to always return a path type if a path type is given in the future... in which case I'll plan to update S3Path as well.

rofinn/FilePathsBase.jl#152

I may change readdir to always return a path type if a path type is given

is there a way to do that while matching Base’s semantics? Like a RelativePath type or something?

I've looked into the RelativePath idea, and it's very verbose. Without a proper traits based approach I don't think that's gonna happen anytime soon. Mostly likely I'd just wrap the resulting string in an appropriate path type and expect subtypes to decide what they want to do. I think there's also an issue where folks want basename to return a path as well. Obviously that change would be very breaking and I'm not clear on the tradeoffs yet, so it probably won't happen right away.

ericphanson added 2 commits January 11, 2021 15:16

add join keyword to readdir

5e64dc6

fix

bcd885b

bors bot added a commit that referenced this pull request Jan 12, 2021

Try #129:

31318f6

ericphanson added 2 commits January 12, 2021 12:35

add version check to tests

5d6fdbd

remove empties correctly

4a8411c

oops! fix < vs >= error

40d986f

bors bot added a commit that referenced this pull request Jan 12, 2021

Try #129:

acac342

fix typo

6896783

bors bot added a commit that referenced this pull request Jan 12, 2021

Try #129:

3709523

rofinn approved these changes Jan 14, 2021

View reviewed changes

src/s3path.jl Outdated Show resolved Hide resolved

src/s3path.jl Outdated Show resolved Hide resolved

apply suggestions from @rofinn's review

debb21c

bors bot merged commit 1f058fc into JuliaCloud:master Jan 15, 2021

ericphanson deleted the eph/readdir_join branch January 15, 2021 18:12

rofinn reviewed Nov 18, 2021

View reviewed changes

rofinn mentioned this pull request Dec 6, 2021

CI test failure: MethodError: no method matching parse(::Type{S3Path{Nothing}}, ::S3Path{Nothing}) #227

Closed

ericphanson mentioned this pull request Jan 4, 2022

Should S3Path stop using the FilePathsBase interface? #226

Open

ericphanson mentioned this pull request Mar 28, 2022

readavailable(::FileBuffer) missing in FilePathsBase < 0.9.18 #245

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `join` keyword to `readdir` #129

add `join` keyword to `readdir` #129

ericphanson commented Jan 11, 2021

mattBrzezinski commented Jan 12, 2021

bors bot commented Jan 12, 2021

mattBrzezinski commented Jan 12, 2021

ericphanson commented Jan 12, 2021

mattBrzezinski commented Jan 12, 2021

bors bot commented Jan 12, 2021

ericphanson commented Jan 12, 2021

mattBrzezinski commented Jan 12, 2021

bors bot commented Jan 12, 2021

ericphanson commented Jan 14, 2021

mattBrzezinski commented Jan 14, 2021

rofinn left a comment

rofinn commented Jan 14, 2021 •

edited

Loading

mattBrzezinski commented Jan 14, 2021

ericphanson commented Jan 15, 2021

mattBrzezinski commented Jan 15, 2021

bors bot commented Jan 15, 2021

rofinn Nov 18, 2021

ericphanson Nov 18, 2021 •

edited

Loading

rofinn Nov 18, 2021

ericphanson Nov 18, 2021 •

edited

Loading

rofinn Nov 18, 2021

ericphanson Nov 18, 2021

rofinn Nov 18, 2021

ericphanson Nov 18, 2021

rofinn Nov 18, 2021 •

edited

Loading

add join keyword to readdir #129

add join keyword to readdir #129

Conversation

ericphanson commented Jan 11, 2021

mattBrzezinski commented Jan 12, 2021

bors bot commented Jan 12, 2021

try

mattBrzezinski commented Jan 12, 2021

ericphanson commented Jan 12, 2021

mattBrzezinski commented Jan 12, 2021

bors bot commented Jan 12, 2021

try

ericphanson commented Jan 12, 2021

mattBrzezinski commented Jan 12, 2021

bors bot commented Jan 12, 2021

try

ericphanson commented Jan 14, 2021

mattBrzezinski commented Jan 14, 2021

rofinn left a comment

Choose a reason for hiding this comment

rofinn commented Jan 14, 2021 • edited Loading

mattBrzezinski commented Jan 14, 2021

ericphanson commented Jan 15, 2021

mattBrzezinski commented Jan 15, 2021

bors bot commented Jan 15, 2021

rofinn Nov 18, 2021

Choose a reason for hiding this comment

ericphanson Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

rofinn Nov 18, 2021

Choose a reason for hiding this comment

ericphanson Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

rofinn Nov 18, 2021

Choose a reason for hiding this comment

ericphanson Nov 18, 2021

Choose a reason for hiding this comment

rofinn Nov 18, 2021

Choose a reason for hiding this comment

ericphanson Nov 18, 2021

Choose a reason for hiding this comment

rofinn Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

add `join` keyword to `readdir` #129

add `join` keyword to `readdir` #129

rofinn commented Jan 14, 2021 •

edited

Loading

ericphanson Nov 18, 2021 •

edited

Loading

ericphanson Nov 18, 2021 •

edited

Loading

rofinn Nov 18, 2021 •

edited

Loading