readdir: add join option to join dir with names
#33113
Conversation
StefanKarpinski
added
needs compat annotation
|
Not 100% sure about the keyword name |
|
Passed everywhere but macOS, failure is unrelated (due to not being able to reach github.com). |
StefanKarpinski
force-pushed
the
sk/readdir
branch
from
c10e8aa
to
1ec9ab4
Compare
StefanKarpinski
removed
needs compat annotation
| @@ -671,41 +671,87 @@ struct uv_dirent_t | |||
| end | |||
|
|
|||
| """ | |||
| readdir(dir::AbstractString=".") -> Vector{String} | |||
| readdir(dir::AbstractString=pwd(); join::Bool=true) -> Vector{String} | |||
There was a problem hiding this comment.
I fixed join=true to join=false (skipping making a PR hoping it would save CI some work, but CI got triggered anyway...)
Also, why did you choose pwd() for dir's default? I guess it makes readdir(join=true) more useful, as otherwise it would be quite equivalent to readdir() (modulo the leading dot), but I think I would still expect . rather than pwd() as the default, as is the case for quite many command line tools.
There was a problem hiding this comment.
Also, why did you choose
pwd()fordir's default? I guess it makesreaddir(join=true)more useful, as otherwise it would be quite equivalent toreaddir()(modulo the leading dot), but I think I would still expect.rather thanpwd()as the default, as is the case for quite many command line tools.
Yes, I did that so that readdir(join=true) would return the absolute paths of everything in the current directory. What difference does using "." versus pwd() for the default make? The only case I can think of where it would matter is if the current working directory has been deleted. Then readdir(".") might work but pwd() would fail. Yeah, now that I write that (and tested the difference), I guess that's what we should do: use "." as the default when join is false and pwd() as the default when join is true.
There was a problem hiding this comment.
What difference does using "." versus pwd() for the default make?
I hadn't even thought about deleted directories, but I meant that I would expect readdir(join=true) to return relative entries of the form ./this.txt. But I'm not convinced either way actually, because it's probably way more common to want the absolute path when you ask join=true.
There was a problem hiding this comment.
Returning a bunch of names with ./ prefixed is pretty useless, whereas returning the absolute paths of all the files in the current directory is useful. If you really want the former, you can always call it as readdir(".", join=true). If you haven't asked for join=true then the only difference between pwd() and "." as a default is the deleted current directory thing, which I'm working on a PR to fix.
There was a problem hiding this comment.
I must admit to not really understand why I argued this point, but I'm glad it allowed you to find the "deleted directory" corner case :)
There was a problem hiding this comment.
I had already thought of it but found it too obscure to bother with, but why not be as thorough as possible: #33175.
129: add `join` keyword to `readdir` r=mattBrzezinski a=ericphanson 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? Co-authored-by: ericphanson <5846501+ericphanson@users.noreply.github.com>
Alternative to #33004.