Implemented deno.readDirSync #652
Conversation
|
@Aaronepower Awesome! I really appreciate you hammering thru all these bindings :) I think we should follow Go in this situation and not just return a list of strings, but a list of FileInfo objects. It's somewhat wasteful to throw away the extra information and only return strings. What do you think? |
|
@ry I was also thinking about that. The one hang up I had is what to add to |
|
@Aaronepower I'm not sure I understand? I would follow Go's os.FileInfo for both statSync and this readDirSync. |
|
@Aaronepower Just a heads up - #659 refactors |
XAMPPRocky
force-pushed
the
master
branch
3 times, most recently
from
d99d124
to
396eaf3
Compare
|
@ry Okay, I've updated the PR with the rebase and with changes so it now returns FileInfos and FileInfos now return paths and file names. |
XAMPPRocky
force-pushed
the
master
branch
2 times, most recently
from
4e8776c
to
6860df4
Compare
js/os.ts
Outdated
| * | ||
| * for (const path of entries) { | ||
| * console.log(path); | ||
| * } |
There was a problem hiding this comment.
Update the example code here for the new API
js/os_test.ts
Outdated
| @@ -100,6 +112,37 @@ test(async function readFileSyncSuccess() { | |||
| assertEqual(pkg.name, "deno"); | |||
| }); | |||
|
|
|||
| test(async function readDirSyncSuccess() { | |||
| const src = deno.readDirSync("src"); | |||
| assert(src.length !== 0); | |||
There was a problem hiding this comment.
We should have a success test that returns non-zero number of entries
XAMPPRocky
force-pushed
the
master
branch
2 times, most recently
from
81c55c4
to
1789bfc
Compare
|
@ry Alright, I've updated with those changes and fixed the merge conflicts with the main branch. |
js/os_test.ts
Outdated
| @@ -34,9 +34,21 @@ test(async function statSyncSuccess() { | |||
| assert(testingInfo.isDirectory()); | |||
| assert(!testingInfo.isSymlink()); | |||
|
|
|||
| const srcInfo = deno.statSync("src"); | |||
| const srcInfo = deno.statSync("src/"); | |||
There was a problem hiding this comment.
Is there a functional reason for "src/" instead of "src"? Also below you use "js/."
... It's an odd way to write these and I'm wondering if you're doing it for extra clarity?
There was a problem hiding this comment.
@ry src/ is just for clarity. js/. is to test that the name of that path is js.
|
@Aaronepower Thanks! Can you rebase and squash? (For some reason github isn't able to do this for me?) |
|
Sorry I should have got this in earlier. But as it is I will wait for #708 to land before this one. |
|
@ry I've updated it to work with the current codebase, however there appears to be a regression in flatbuffers' Rust codegen, where it won't declare a lifetime for the |
|
@Aaronepower hey - sorry about this but this is the oldest PR here now and it's red so I can't merge it. I also don't want to dig in and fix it myself at the moment. So I'd be extremely grateful if you rebased and resubmitted - or if not - I will come back and rebase your code after landing #782. (I'll cite you as the author, of course). |
Node:
fs.readdirSyncreturns a list of paths of entries in the path.Rust:
fs::read_dirreturns an iterator ofDirEntrys.Go:
ioutil.ReadDirreturns a list ofos.FileInfos.Ruby:
Diris a class with various methods for general directory operations such as changing directories.Dir["dir/"]returns a array of strings matching the path. This also accepts a general glob pattern as opposed to a strict path.Python:
os.listdirreturns an list of paths in arbitrary order.