feat(fs): Make parameters optional for readSync#32460

lholmquist · 2020-03-24T15:33:06Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Similar to the recently merged #31402, which was for fs.read, this PR does something similar for fs.readSync

This adds the signature fs.readSync(fd, buffer, options), where the options is an object that can contain optional values for offset, length and position.

The difference with this PR and the previous one for fs.read, is that here i'm making the buffer object not optional. Since the function returns the amount of bytes read, the user needs to use the buffer they pass in to actually read value of what they are trying to read.

I still need to add docs for this, but wanted to get any thoughts on if also making buffer optional and part of the options object which would then change the new function signature to fs.readSync(fd,{})

test/parallel/test-fs-readSync-optional-params.js

mmarchini · 2020-03-24T18:24:52Z

Hi @lholmquist. Do you mind updating the commit message to follow our commit guidelines? We don't use feat(), and the message following the subsystem should start with lowercase, so the commit message title here should be: fs: make parameters optional for readSync

lholmquist · 2020-03-24T18:26:43Z

@mmarchini yea, no problem. I should've known that already since this isn't my first contribution :)

mmarchini · 2020-03-24T18:27:47Z

No worries, it's easy to forget some details from the guideline :)

himself65 · 2020-03-25T02:12:14Z

LGTM if update doc and fix the following suggested changes.

optional: edit the PR title to same with the commit

lib/fs.js

addaleax · 2020-03-29T19:19:56Z

@himself65 Can you take another look?

nodejs-github-bot · 2020-03-29T19:20:31Z

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

addaleax · 2020-03-30T15:05:50Z

@lholmquist Sorry it took so long to get back to you, but it seems like this conflicts with recent changes. Could you rebase this?

lholmquist · 2020-03-30T15:24:35Z

@addaleax no problem. rebase commencing

* This makes the offset, length and position parameters optional by passing in an options object.

nodejs-github-bot · 2020-03-30T17:33:46Z

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

nodejs-github-bot · 2020-03-30T18:11:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30269/ (:heavy_check_mark:)

This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <[email protected]>

addaleax · 2020-04-02T16:39:14Z

Landed in 88b4e86 🙂

This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <[email protected]>

Notable changes: New file system APIs: * Added a new function, `fs.readv` (with sync and promisified versions). This function takes an array of `ArrayBufferView` elements and will write the data it reads sequentially to the buffers (Sk Sajidul Kadir). #32356 * A new overload is available for `fs.readSync`, which allows to optionally pass any of the `offset`, `length` and `position` parameters. #32460 Other changes: * dns: * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4 mapped IPv6 addresses (murgatroid99). #32183 * http: * The default maximum HTTP header size was changed from 8KB to 16KB (rosaxny). #32520 * n-api: * Calls to `napi_call_threadsafe_function` from the main thread can now return the `napi_would_deadlock` status in certain circumstances (Gabriel Schulhof). #32689 * util: * Added a new `maxStrLength` option to `util.inspect`, to control the maximum length of printed strings. Its default value is `Infinity` (rosaxny). #32392 * worker: * Added support for passing a `transferList` along with `workerData` to the `Worker` constructor (Juan José Arboleda). #32278 New core collaborators: With this release, we welcome three new Node.js core collaborators: * himself65. #32734 * flarna (Gerhard Stoebich). #32620 * mildsunrise (Alba Mendez). #32525 PR-URL: #32813

This makes the offset, length and position parameters optional by passing in an options object. PR-URL: nodejs#32460 Reviewed-By: Anna Henningsen <[email protected]>

This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Mar 24, 2020

himself65 requested changes Mar 24, 2020
View reviewed changes

test/parallel/test-fs-readSync-optional-params.js Outdated Show resolvedHide resolved

himself65 requested changes Mar 24, 2020
View reviewed changes

test/parallel/test-fs-readSync-optional-params.js Outdated
Comment on lines 11 to 34
This comment was marked as outdated.
Sign in to view
This comment was marked as outdated.
Sign in to view

lholmquist force-pushed the fs-read-sync-optional-params branch from 3cb5c23 to bc32a48Compare March 24, 2020 18:28

himself65 requested changes Mar 25, 2020
View reviewed changes

lib/fs.js Outdated Show resolvedHide resolved

lholmquist force-pushed the fs-read-sync-optional-params branch from bc32a48 to d8b91e6Compare March 25, 2020 18:27

addaleax approved these changes Mar 29, 2020
View reviewed changes

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 29, 2020

himself65 approved these changes Mar 30, 2020
View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 30, 2020

lholmquist added 4 commits March 30, 2020 11:25

fs: Make parameters optional for readSync
c91f09e
* This makes the offset, length and position parameters optional by passing in an options object.

squash: fix typo
7115ba9

squash: fix up tests
c2bb51b

squash: update docs
7f68e77

lholmquist force-pushed the fs-read-sync-optional-params branch from fb855f8 to 7f68e77Compare March 30, 2020 15:31

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 30, 2020

addaleax pushed a commit that referenced this pull request Apr 2, 2020
fs: make parameters optional for readSync
88b4e86
This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <[email protected]>

addaleax closed this Apr 2, 2020

BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
fs: make parameters optional for readSync
6d426bb
This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <[email protected]>

targos pushed a commit that referenced this pull request Apr 12, 2020
fs: make parameters optional for readSync
991aca3
This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <[email protected]>

targos mentioned this pull request Apr 13, 2020
v13.13.0 release proposal #32813
Merged

targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Apr 25, 2020

targos pushed a commit that referenced this pull request Apr 28, 2020
fs: make parameters optional for readSync
a100709
This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <[email protected]>

lholmquist deleted the fs-read-sync-optional-params branch April 29, 2020 14:33

targos mentioned this pull request May 2, 2020
v12.17.0 release proposal #33197
Merged

Uh oh!

feat(fs): Make parameters optional for readSync#32460

feat(fs): Make parameters optional for readSync #32460

Uh oh!

Conversation

lholmquist commented Mar 24, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

mmarchini commented Mar 24, 2020

Uh oh!

lholmquist commented Mar 24, 2020

Uh oh!

mmarchini commented Mar 24, 2020

Uh oh!

himself65 commented Mar 25, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

addaleax commented Mar 29, 2020

Uh oh!

nodejs-github-bot commented Mar 29, 2020

Uh oh!

addaleax commented Mar 30, 2020

Uh oh!

lholmquist commented Mar 30, 2020

Uh oh!

nodejs-github-bot commented Mar 30, 2020

Uh oh!

nodejs-github-bot commented Mar 30, 2020• edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Apr 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lholmquist commented Mar 24, 2020•
edited
Loading

himself65 commented Mar 25, 2020•
edited
Loading

nodejs-github-bot commented Mar 30, 2020•
edited by addaleax
Loading