Skip to content

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jan 3, 2017

Allow fs.read, fs.write and fs.writeFile to take
Uint8Array arguments.

PR-URL: #10382

The only difference from the original commit is the added line in node_util.cc

/cc @evanlucas

Allow `fs.read`, `fs.write` and `fs.writeFile` to take
`Uint8Array` arguments.

PR-URL: nodejs#10382
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
@addaleax addaleax added 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. v7.x labels Jan 3, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. util Issues and PRs related to the built-in util module. v7.x labels Jan 3, 2017
@addaleax
Copy link
Member Author

addaleax commented Jan 3, 2017

@addaleax addaleax removed the util Issues and PRs related to the built-in util module. label Jan 3, 2017
0,
expected.length,
0,
common.mustCall((err, bytesRead) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you assert the error here?

fs.closeSync(fd);

const found = fs.readFileSync(filename, 'utf8');
assert.deepStrictEqual(expected.toString(), found);
Copy link
Contributor

Choose a reason for hiding this comment

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

Strings don't need deep comparison, right?

@addaleax
Copy link
Member Author

addaleax commented Jan 3, 2017

@thefourtheye I’ll be happy to address your suggestions, but I’d prefer to do that on master instead of in a backport if that’s okay. :)

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

Thanks!!

Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

@addaleax Sure, no problem. LGTM, if the CI is happy.

@addaleax
Copy link
Member Author

addaleax commented Jan 4, 2017

@evanlucas I guess whether you want to have this in #10589 is your call? I don’t want to step on anybody’s toes by landing this at the wrong time

@evanlucas
Copy link
Contributor

@addaleax feel free to land it. I was planning on bringing it any anyways :]

@addaleax addaleax merged commit abde764 into nodejs:v7.x-staging Jan 4, 2017
@addaleax addaleax deleted the backport-v7.x-10382 branch January 4, 2017 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. 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.

4 participants