fs: add default options for stat, fstat, and lstat #29114
Conversation
|
@nodejs/fs |
| } | ||
|
|
||
| function fstat(fd, options, callback) { | ||
| function fstat(fd, options = { bigint: false }, callback) { |
There was a problem hiding this comment.
Why not just options = {}? (Same below.)
There was a problem hiding this comment.
I just copied fs/promises.js
node/lib/internal/fs/promises.js
Line 339 in 427e534
Should I use getOptions(options, {bigint: false}) to match the other functions?
There was a problem hiding this comment.
getOptions() does a bunch of other things that do not fit these APIs (like accepting a string as the encoding option), using default arguments should be fine considering we do this for many other APIs as well.
|
I seem to recall that we used to avoid default options like this because it would run slower than doing the equivalent in the first few lines of the function. But I don't know if that's no longer a problem or not (or if it never was an I'm imagining it). /ping @mscdex |
|
Welcome @UziTech and thanks for the pull request! |
|
@joyeecheung I am not very familiar with the node codebase. Where would you like me to add the tests? |
|
I think I found the right place. I added the tests to |
|
I don't think the failed tests have anything to do with my code changes. |
|
Landed in 4111c57. Thanks for the contribution! 🎉 |
Add default options to the function signatures for fs.stat, fs.fstat, and fs.lstat.
This allows a library to pass undefined to get the default options.
Fixes: #29113
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes