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: use consistent defaults in sync stat functions #31097

Closed
wants to merge 1 commit into from
Closed

fs: use consistent defaults in sync stat functions #31097

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 26, 2019

This commit updates the default options used by statSync(), lstatSync(), and fstatSync() to be identical to the defaults used by the callback- and Promise-based versions.

Technically, the binding layer treats bigint values of undefined and false the same, but we might as well be consistent.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@cjihrig
fs: use consistent defaults in sync stat functions
522cbe8 
This commit updates the default options used by statSync(),
lstatSync(), and fstatSync() to be identical to the defaults
used by the callback- and Promise-based versions.
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Dec 26, 2019
devsnek
devsnek approved these changes Dec 26, 2019
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

this means that xyz(path, {}) will still have options.bigint be undefined. we may want to explore defaults further in future changes.

ZYSzys
ZYSzys reviewed Dec 26, 2019
View reviewed changes
lib/fs.js
@@ -922,15 +922,15 @@ function stat(path, options = { bigint: false }, callback) {
binding.stat(pathModule.toNamespacedPath(path), options.bigint, req);
}

function fstatSync(fd, options = {}) {
function fstatSync(fd, options = { bigint: false }) {
validateInt32(fd, 'fd', 0);
const ctx = { fd };
const stats = binding.fstat(fd, options.bigint, undefined, ctx);
Copy link
Member

Choose a reason for hiding this comment

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

For @devsnek 's concerns, how about changing this to:

Suggested change
const stats = binding.fstat(fd, options.bigint, undefined, ctx);
const stats = binding.fstat(fd, options.bigint || false, undefined, ctx);

Copy link
Member

Choose a reason for hiding this comment

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

in a future change we could do { bigint = false } = {} for all of them

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 28, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 28, 2019 •
edited by cjihrig
Loading

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

@Trott
Copy link
Member

Trott commented Dec 29, 2019

Landed in 3cd7780

@Trott Trott closed this Dec 29, 2019
Trott pushed a commit that referenced this pull request Dec 29, 2019
@cjihrig @Trott
fs: use consistent defaults in sync stat functions
3cd7780 
This commit updates the default options used by statSync(),
lstatSync(), and fstatSync() to be identical to the defaults
used by the callback- and Promise-based versions.

PR-URL: #31097
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@cjihrig cjihrig deleted the stat-defaults branch December 29, 2019 15:47
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
@cjihrig @BridgeAR
fs: use consistent defaults in sync stat functions
4f32bbb 
This commit updates the default options used by statSync(),
lstatSync(), and fstatSync() to be identical to the defaults
used by the callback- and Promise-based versions.

PR-URL: #31097
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@cjihrig @targos
fs: use consistent defaults in sync stat functions
17a48f1 
This commit updates the default options used by statSync(),
lstatSync(), and fstatSync() to be identical to the defaults
used by the callback- and Promise-based versions.

PR-URL: #31097
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@cjihrig @BethGriggs
fs: use consistent defaults in sync stat functions
465a1cf 
This commit updates the default options used by statSync(),
lstatSync(), and fstatSync() to be identical to the defaults
used by the callback- and Promise-based versions.

PR-URL: #31097
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys left review comments

@devsnek devsnek devsnek approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
9 participants
@cjihrig @nodejs-github-bot @Trott @jasnell @addaleax @targos @devsnek @gireeshpunathil @ZYSzys