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

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

Closed

Conversation

lholmquist
Copy link
Contributor

@lholmquist lholmquist commented Mar 24, 2020

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, {})

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Mar 24, 2020
Comment on lines 11 to 34
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);

This comment was marked as outdated.

This comment was marked as outdated.

@mmarchini
Copy link
Contributor

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
Copy link
Contributor Author

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

@mmarchini
Copy link
Contributor

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

@himself65
Copy link
Member

himself65 commented Mar 25, 2020

LGTM if update doc and fix the following suggested changes.

optional: edit the PR title to same with the commit

lib/fs.js Outdated Show resolved Hide resolved
@addaleax 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
@addaleax
Copy link
Member

@himself65 Can you take another look?

@nodejs-github-bot
Copy link
Collaborator

@addaleax 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
@addaleax
Copy link
Member

@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
Copy link
Contributor Author

@addaleax no problem. rebase commencing

* This makes the offset, length and position parameters optional by
passing in an options object.
@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 30, 2020

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

addaleax pushed a commit that referenced this pull request Apr 2, 2020
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
Copy link
Member

addaleax commented Apr 2, 2020

Landed in 88b4e86 🙂

@addaleax addaleax closed this Apr 2, 2020
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
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
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 added a commit that referenced this pull request Apr 14, 2020
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
targos added a commit that referenced this pull request Apr 14, 2020
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
targos added a commit that referenced this pull request Apr 14, 2020
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
@targos 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 to targos/node that referenced this pull request Apr 25, 2020
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]>
targos pushed a commit that referenced this pull request Apr 28, 2020
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 lholmquist deleted the fs-read-sync-optional-params branch April 29, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants