Add more dir api for Deno #3491
Conversation
|
/cc @ry it's ready for review |
cli/ops/os.rs
Outdated
| "public" => dirs::public_dir(), | ||
| "template" => dirs::template_dir(), | ||
| "video" => dirs::video_dir(), | ||
| _ => None, |
There was a problem hiding this comment.
No test cases needed here.
I change this to panic when not match.
So if not match the case, an exception will be thrown and the CI won't pass
There was a problem hiding this comment.
I think it should be tested to confirm it returns an error (not panics)
There was a problem hiding this comment.
Now it returns an error
But I'm not sure where this test case should be written
Maybe cli/js/os_test.ts or cli/js/dispatch_json.ts?
|
I think it’s better not to panic - because users can still trigger that by calling the op directly. Better to return Err(...) I’m not at my computer to give a good example but you should be able to look through the other ops and find an example of returning an error. |
cli/js/os.ts
Outdated
| const path = sendSync(dispatch.OP_GET_DIR, { name: "cache" }); | ||
| if (!path) { | ||
| throw new Error("Could not get user cache directory."); | ||
| } |
There was a problem hiding this comment.
All of these places where you throw new Error can be removed if you return the error from the op.
|
@ry |
close #3488
depend on https://crates.io/crates/dirs
If the directory does not exist, an exception is thrown
E.g.
Deno.downloadDir()Not every Linux has a download directory
TODO: