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.

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: default open/openSync flags argument to 'r' #23767

Closed
wants to merge 4 commits into from

Conversation

bnoordhuis
Copy link
Member

Make fs.open() and fs.openSync() more economic to use by making the
flags argument optional. You can now write:

fs.open(file, cb)

Instead of the more verbose:

fs.open(file, 'r', cb)

This idiom is already supported by functions like fs.readFile().

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 19, 2018
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM. What would the semver-iness of this be? minor?

doc/api/fs.md Outdated
@@ -2318,7 +2318,8 @@ changes:
-->

* `path` {string|Buffer|URL}
* `flags` {string|number} See [support of file system `flags`][].
* `flags` {string|number} **Default:** `'r'`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nits:

  1. Missing period.
  2. One unneeded space in the next line indent alignment,
  3. It seems our common style is to place **Default:** notes at the end of a parameter description:
* `flags` {string|number} See [support of file system `flags`][].
  **Default:** `'r'`.

doc/api/fs.md Outdated
@@ -2351,7 +2352,8 @@ changes:
-->

* `path` {string|Buffer|URL}
* `flags` {string|number} See [support of file system `flags`][].
* `flags` {string|number} **Default:** `'r'`
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto:

* `flags` {string|number} See [support of file system `flags`][].
  **Default:** `'r'`.

@vsemozhetbyt
Copy link
Contributor

Should the fsPromises.open(path, flags[, mode]) be unified?

doc/api/fs.md Outdated
@@ -2304,7 +2304,7 @@ this API: [`fs.mkdtemp()`][].
The optional `options` argument can be a string specifying an encoding, or an
object with an `encoding` property specifying the character encoding to use.

## fs.open(path, flags[, mode], callback)
## fs.open(path[, flags, mode], callback)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## fs.open(path[, flags, mode], callback)
## fs.open(path[, flags[, mode]], callback)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a style we use elsewhere? It looks unfamiliar to me.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The [, flags[, mode]], is the right doc convention, and looks to agree with how it is now implemented.

Make fs.open() and fs.openSync() more economic to use by making the
flags argument optional. You can now write:

    fs.open(file, cb)

Instead of the more verbose:

    fs.open(file, 'r', cb)

This idiom is already supported by functions like fs.readFile().
@bnoordhuis
Copy link
Member Author

Updated based on feedback:

  • doc nits
  • updated fs.promises.open()

@refack refack added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 23, 2018
@refack
Copy link
Contributor

refack commented Oct 23, 2018

Is this semver minor or major? i.e. could any existing code break?

CI: https://ci.nodejs.org/job/node-test-pull-request/18088/
Resume: https://ci.nodejs.org/job/node-test-pull-request/18111/

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2018
@bnoordhuis
Copy link
Member Author

What would the semver-iness of this be? minor?

Yes, minor.

@bnoordhuis
Copy link
Member Author

Test failure is #23807 and unrelated (wasn't fixed yet when the CI was started.)

@Trott
Copy link
Member

Trott commented Oct 26, 2018

Last CI was green. Any reason this shouldn't land?

@lpinca
Copy link
Member

lpinca commented Oct 26, 2018

I think this can land, my nit can be addressed in another PR if wanted.

@Trott
Copy link
Member

Trott commented Oct 26, 2018

Applied the nit.

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

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 26, 2018
Make fs.open() and fs.openSync() more economic to use by making the
flags argument optional. You can now write:

    fs.open(file, cb)

Instead of the more verbose:

    fs.open(file, 'r', cb)

This idiom is already supported by functions like fs.readFile().

PR-URL: nodejs#23767
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 26, 2018

Landed in 482b56a

@Trott Trott closed this Oct 26, 2018
targos pushed a commit that referenced this pull request Oct 27, 2018
Make fs.open() and fs.openSync() more economic to use by making the
flags argument optional. You can now write:

    fs.open(file, cb)

Instead of the more verbose:

    fs.open(file, 'r', cb)

This idiom is already supported by functions like fs.readFile().

PR-URL: #23767
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Nov 1, 2018
Make fs.open() and fs.openSync() more economic to use by making the
flags argument optional. You can now write:

    fs.open(file, cb)

Instead of the more verbose:

    fs.open(file, 'r', cb)

This idiom is already supported by functions like fs.readFile().

PR-URL: #23767
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg added a commit to rvagg/io.js that referenced this pull request Nov 12, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 12, 2018
Was missed on original PR.

Ref: nodejs#23767

PR-URL: nodejs#24240
Refs: nodejs#23767
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Was missed on original PR.

Ref: #23767

PR-URL: #24240
Refs: #23767
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Was missed on original PR.

Ref: nodejs#23767

PR-URL: nodejs#24240
Refs: nodejs#23767
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Dec 14, 2018
Was missed on original PR.

Ref: #23767

PR-URL: #24240
Refs: #23767
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Was missed on original PR.

Ref: #23767

PR-URL: #24240
Refs: #23767
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
Make fs.open() and fs.openSync() more economic to use by making the
flags argument optional. You can now write:

    fs.open(file, cb)

Instead of the more verbose:

    fs.open(file, 'r', cb)

This idiom is already supported by functions like fs.readFile().

PR-URL: #23767
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants