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

+const defaultBuffer1 = Buffer.allocUnsafe(expected.length);
+const defaultBuffer2 = Buffer.allocUnsafe(expected.length);
+const defaultBuffer3 = Buffer.allocUnsafe(expected.length);
+
+// Test passing in an empty options object
+const result1 = fs.readSync(fd, defaultBuffer1, { position: 0 });
+assert.strictEqual(result1, expected.length);
+assert.deepStrictEqual(defaultBuffer1, expected);
+
+// Test not passing in any options object
+const result2 = fs.readSync(fd, defaultBuffer2);
+assert.strictEqual(result2, expected.length);
+assert.deepStrictEqual(defaultBuffer2, expected);
+
+
+// Test passing in options
+const result3 = fs.readSync(fd, defaultBuffer3, {
+  offset: 0,
+  length: defaultBuffer3.length,
+  position: 0
+});
+
+assert.strictEqual(result3, expected.length);
+assert.deepStrictEqual(defaultBuffer3, expected);


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 <anna@addaleax.net>

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 <anna@addaleax.net>

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 <anna@addaleax.net>

This makes the offset, length and position parameters optional by passing in an options object. PR-URL: #32460 Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 resolved Hide resolved

himself65 requested changes Mar 24, 2020

View reviewed changes

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

himself65 requested changes Mar 25, 2020

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

lholmquist force-pushed the fs-read-sync-optional-params branch from bc32a48 to d8b91e6 Compare 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 7f68e77 Compare 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 <anna@addaleax.net>

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 <anna@addaleax.net>

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 <anna@addaleax.net>

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 <anna@addaleax.net>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lholmquist commented Mar 24, 2020 •

edited

This comment was marked as outdated.

This comment was marked as outdated.

mmarchini commented Mar 24, 2020

lholmquist commented Mar 24, 2020

mmarchini commented Mar 24, 2020

himself65 commented Mar 25, 2020 •

edited

addaleax commented Mar 29, 2020

nodejs-github-bot commented Mar 29, 2020

addaleax commented Mar 30, 2020

lholmquist commented Mar 30, 2020

nodejs-github-bot commented Mar 30, 2020

nodejs-github-bot commented Mar 30, 2020 •

edited by addaleax

addaleax commented Apr 2, 2020

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

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

Conversation

lholmquist commented Mar 24, 2020 • edited

Checklist

This comment was marked as outdated.

This comment was marked as outdated.

mmarchini commented Mar 24, 2020

lholmquist commented Mar 24, 2020

mmarchini commented Mar 24, 2020

himself65 commented Mar 25, 2020 • edited

addaleax commented Mar 29, 2020

nodejs-github-bot commented Mar 29, 2020

addaleax commented Mar 30, 2020

lholmquist commented Mar 30, 2020

nodejs-github-bot commented Mar 30, 2020

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

addaleax commented Apr 2, 2020

lholmquist commented Mar 24, 2020 •

edited

himself65 commented Mar 25, 2020 •

edited

nodejs-github-bot commented Mar 30, 2020 •

edited by addaleax