-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
refactor(ext/fs): align error messages #25414
Conversation
Aligns the error messages in the ext/fs folder to be in-line with the Deno style guide. denoland#25269
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, left a comment. Let me know what you think
ext/fs/30_fs.js
Outdated
@@ -776,11 +776,15 @@ function checkOpenOptions(options) { | |||
(val) => val === true, | |||
).length === 0 | |||
) { | |||
throw new Error("OpenOptions requires at least one option to be true"); | |||
throw new Error( | |||
"Cannot open file: `options` requires at least one option to be true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use the same quoting style as the other changes in this PR?
"Cannot open file: `options` requires at least one option to be true", | |
"Cannot open file: 'options' requires at least one option to be true", |
That said I'm not sure about the wording for these messages. The error is thrown before it was attempted to open the file, so the Cannot open file
bit is confusing to me. It's good that we're hinting as to which fs option was called. Maybe having that in the stack trace is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the quoting styles. I've noticed a real mixed bag, let's go with single quotes like you have here.
As for the error message, the checkOpenOption
seems to only be used during the open
and openSync
functions, so from a callers perspective, the "File could not be opened" because of this error. I'm happy to drop it though it that's confusing, and of course the stack trace will be there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the "Cannot open file" prefix and aligned the quoting styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Aligns the error messages in the ext/fs folder to be in-line with the Deno style guide.
#25269