Skip to content

fs: add default options for stat, fstat, and lstat #29114

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

fs: add default options for stat, fstat, and lstat #29114

wants to merge 2 commits into from

Conversation

UziTech
Copy link
Contributor

@UziTech UziTech commented Aug 14, 2019 •
edited
Loading

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 14, 2019
@Trott
Copy link
Member

Trott commented Aug 14, 2019

@nodejs/fs

Trott
Trott reviewed Aug 14, 2019
View reviewed changes
lib/fs.js
@@ -796,7 +796,7 @@ function readdirSync(path, options) {
return options.withFileTypes ? getDirents(path, result) : result;
}

function fstat(fd, options, callback) {
function fstat(fd, options = { bigint: false }, callback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just options = {}? (Same below.)

Copy link
Contributor Author

@UziTech UziTech Aug 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied fs/promises.js

node/lib/internal/fs/promises.js

Line 339 in 427e534

async function fstat(handle, options = { bigint: false }) {

Should I use getOptions(options, {bigint: false}) to match the other functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Trott
Copy link
Member

Trott commented Aug 14, 2019

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

@Trott
Copy link
Member

Trott commented Aug 14, 2019

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/430/

@Trott
Copy link
Member

Trott commented Aug 14, 2019

Welcome @UziTech and thanks for the pull request!

joyeecheung
joyeecheung reviewed Aug 14, 2019
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

@UziTech
Copy link
Contributor Author

UziTech commented Aug 14, 2019

@joyeecheung I am not very familiar with the node codebase. Where would you like me to add the tests?

@UziTech
Copy link
Contributor Author

UziTech commented Aug 14, 2019

I think I found the right place. I added the tests to /test/parallel/test-fs-stat.js

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/24985/

@UziTech
Copy link
Contributor Author

UziTech commented Aug 15, 2019

I don't think the failed tests have anything to do with my code changes.

@UziTech UziTech mentioned this pull request Aug 15, 2019
Closed
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/25024/

@Trott
Copy link
Member

Trott commented Aug 16, 2019

Landed in 4111c57.

Thanks for the contribution! 🎉

@Trott Trott closed this Aug 16, 2019
Trott pushed a commit that referenced this pull request Aug 16, 2019
@UziTech @Trott
fs: add default options for *stat()
4111c57 
PR-URL: #29114
Fixes: #29113
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Aug 19, 2019
@UziTech @targos
fs: add default options for *stat()
178caa5 
PR-URL: #29114
Fixes: #29113
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Aug 19, 2019
Merged
This was referenced Aug 20, 2019
Closed
Closed
Closed
Closed
This was referenced Aug 20, 2019
Closed
Closed
@UziTech UziTech deleted the patch-1 branch February 11, 2020 16:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@patilharshal16 patilharshal16 patilharshal16 approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot read property 'bigint' of undefined
7 participants
@UziTech @Trott @nodejs-github-bot @jasnell @lpinca @joyeecheung @patilharshal16