Skip to content
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.

Sign up for GitHub

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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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