feat(fs): Make parameters optional for readSync - #32460
Closed
lholmquist wants to merge 4 commits into
Closed
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
himself65
requested changes
Mar 24, 2020
Comment thread
test/parallel/test-fs-readSync-optional-params.js
Outdated
himself65
requested changes
Mar 24, 2020
Comment thread
test/parallel/test-fs-readSync-optional-params.js
Outdated
Comment on lines
11
to
34
This comment was marked as outdated.
This comment was marked as outdated.
Copy link
Copy Markdown
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 |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
@mmarchini yea, no problem. I should've known that already since this isn't my first contribution :) |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
No worries, it's easy to forget some details from the guideline :) |
Sorry, something went wrong.
lholmquist
force-pushed
the
fs-read-sync-optional-params
branch
from
3cb5c23 to
bc32a48
Compare
March 24, 2020 18:28
Copy link
Copy Markdown
Member
|
LGTM if update doc and fix the following suggested changes. optional: edit the PR title to same with the commit |
Sorry, something went wrong.
himself65
requested changes
Mar 25, 2020
Comment thread
lib/fs.js
Outdated
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
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
himself65
approved these changes
Mar 30, 2020
Copy link
Copy Markdown
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? |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
@addaleax no problem. rebase commencing |
Sorry, something went wrong.
* This makes the offset, length and position parameters optional by passing in an options object.
lholmquist
force-pushed
the
fs-read-sync-optional-params
branch
from
fb855f8 to
7f68e77
Compare
March 30, 2020 15:31
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
|
CI: https://ci.nodejs.org/job/node-test-pull-request/30269/ (:heavy_check_mark:) |
Sorry, something went wrong.
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 <anna@addaleax.net>
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 <anna@addaleax.net>
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 <anna@addaleax.net>
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
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 <anna@addaleax.net>
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 <anna@addaleax.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
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, {})