-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
fs: ensure return value type of options.filter
in cpSync
consistent with doc
#52461
Conversation
Can you add a test? |
Hi, I have added related test |
if (isPromise(shouldCopy)) { | ||
if (typeof shouldCopy !== 'boolean') { |
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 have the feeling this was done on purpose, to support truthy values, and there's a Promise check to prevent mistakes.
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.
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.
Thanks for your feedback. and I have a question, for
opt.filter = (value) => {
//
}
its return type contradicts the documentation, which states Returns: <boolean>
, but it wont throw an error. Should here use a more precise description, such as the type that is coercible to boolean type
?
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.
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'm fine with either (changing the code or the docs); if we change the code, let's land this as semver-major.
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 think I might prefer changing the docs to changing the code here.
Closed in favor of #52742. |
PR-URL: nodejs#52742 Refs: nodejs#52461 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs#52742 Refs: nodejs#52461 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Ensure return type of options.filter in cpsync matches doc.
according to the documentation, the return value of
options.filter
hasboolean
type.