close
The Wayback Machine - https://web.archive.org/web/20210811152650/https://github.com/denoland/deno/issues/10553
Skip to content

/ deno

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

Jump to bottom

Improve the error message from stat/statSync #10553

Open
canonic-epicure opened this issue May 10, 2021 · 8 comments
Open

Improve the error message from stat/statSync #10553

canonic-epicure opened this issue May 10, 2021 · 8 comments
Labels
good first issue

Comments

@canonic-epicure
Copy link

@canonic-epicure canonic-epicure commented May 10, 2021

Currently the error message of the exception, thrown from the Deno.stat/statSync, when the file is missing, does not include the file name:

nickolay@frontier:~/workspace/Bryntum/siesta-monorepo/siesta$ deno
Deno 1.9.2
exit using ctrl+d or close()
> Deno.statSync('/home/nickolay/not_existing_file')
Uncaught NotFound: No such file or directory (os error 2)
    at unwrapOpResult (deno:core/core.js:99:13)
    at Object.opSync (deno:core/core.js:113:12)
    at Object.statSync (deno:runtime/js/30_fs.js:224:22)
    at <anonymous>:2:6
>

Is this intentional with some rationale behind, or just an oversight? From usability perspective the filename should be included in the message of course.
@bnoordhuis
Copy link
Contributor

@bnoordhuis bnoordhuis commented May 12, 2021

Pull request welcome!

The error message is bubbled up straight from the Rust runtime. The relevant code is here:

deno/runtime/ops/fs.rs

Lines 846 to 850 in 1cd1419

let metadata = if lstat {
std::fs::symlink_metadata(&path)?
} else {
std::fs::metadata(&path)?
};

It's possible to remap the error message to include the filename but it should be done in a principled manner, i.e., for more than just stat.

With ops that operate on more than one path (like link) you don't know which path is the problematic one. I'd include both in the error message, that's what Node does.
@CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented May 15, 2021 •
edited

image

here's how the node prints out the error message, should I print it just like that?
@CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented May 15, 2021

image

Is this ok or not? @bnoordhuis
@CGQAQ CGQAQ mentioned this issue May 15, 2021
Closed
@exclowd
Copy link

@exclowd exclowd commented Jun 29, 2021

@CGQAQ if you are not working on this, can I take this up?
@CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Jun 29, 2021

@bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Jun 30, 2021

@CGQAQ if you are not working on this, can I take this up?

Please do, there's already an open PR #10642, but ideally we'd want to unify error messages for all ops in ops/fs.rs and provide the paths in those messages.
@caspervonb caspervonb mentioned this issue Jul 1, 2021
Closed
@ntedgi
Copy link

@ntedgi ntedgi commented Jul 27, 2021

Is this PR still available?
I would be happy to give it a try
@exclowd
Copy link

@exclowd exclowd commented Jul 27, 2021

Sure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Added path info to error message from stat/statSync
6 participants
@canonic-epicure @bnoordhuis @bartlomieju @CGQAQ @ntedgi @exclowd