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

Rename and publicize Base.Filesystem._readdirx #55333

Open
LilithHafner opened this issue Aug 1, 2024 · 9 comments · May be fixed by #55358 or #55393
Open

Rename and publicize Base.Filesystem._readdirx #55333

LilithHafner opened this issue Aug 1, 2024 · 9 comments · May be fixed by #55358 or #55393
Labels
design Design of APIs or of the language itself

Comments

@LilithHafner
Copy link
Member

We added _readdirx with an internal name to defer bikeshedding. Here's an issue for that bikeshed.

@LilithHafner LilithHafner added the design Design of APIs or of the language itself label Aug 1, 2024
@LilithHafner
Copy link
Member Author

Triage talked about this and came up with the following:

  • readdir(DirEntry, path)

    • More discoverable on ?readdir
    • Extensible, e.g. readdir(Path, path) for a package that defines paths
    • Julia is all about shoving things into functions
    • Follows parse and JuliaSyntax.parse
  • readdirentry(path)

    • More clear that it is different functionality

Nobody at triage wanted these names:

  • readdirx(path)
  • ls(path)
  • parse(Vector{DirEntry}, path)

@ericphanson
Copy link
Contributor

it would be nice if there was an abstraction also usable by S3, i.e. some readdir/walkdir style function that when used with s3 can give you back an iterable of objects which knows the metadata that we got for-free from list_objects_v2() or such (i.e. key, size, last modified, etag, etc).

I think readdir(DirEntry, path) could work for that where AWSS3 could define readdir(S3Entry, path::S3Path).

@oscardssmith
Copy link
Member

I think readdir(S3Entry, path::S3Path) is a pretty great example of why readdir(DirEntry, path) is the way to go.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 1, 2024

I feel like that is actually an argument against that API. The idea that I need to write readdir(typeof(path), path) seems quite strange to me.

@IanButterworth
Copy link
Sponsor Member

typeof(path), path seems like the wrong example. You don't want typeof(path) you want an "Entry" type, so readdir(S3Entry, path::S3Path) makes sense to me, assuming that readdir(path::S3Path) returns a list of Strings already.

@ericphanson
Copy link
Contributor

readdir(path::S3Path) indeed returns a list of strings, while readdir(path::S3Path; join=true) returns a list of S3Paths.

@LilithHafner
Copy link
Member Author

readdir(path::S3Path; join::Bool) is type unstable. It could be made type stable if it used two a type argument to determine return type rather than the value of join.

@ericphanson
Copy link
Contributor

ericphanson commented Aug 1, 2024

yes it is type unstable, but that seems necessary for join=true to have the semantic that you can operate on the outputs of readdir(path; join=true) with e.g. isfile, read, etc, which is the useful property of join=true, and the network call will likely be in ms compared to the microseconds of dynamic dispatch anyway. One could also hope that const-prop could eliminate the instability.

Anyway I bring it up here as a wart on the existing readdir design for non-string paths, with the hope that the design of this feature can keep them in mind a bit better.

@IanButterworth IanButterworth linked a pull request Aug 3, 2024 that will close this issue
@IanButterworth
Copy link
Sponsor Member

Realized here #55358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design of APIs or of the language itself
Projects
None yet
5 participants