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

Update fs promise shims to support latest Node functions #747

Merged
merged 4 commits into from
Feb 3, 2020

Conversation

RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Jan 31, 2020

Added:

  • fs.writev
  • fs.opendir

@RyanZim RyanZim added this to the 9.0.0 milestone Jan 31, 2020
@RyanZim RyanZim requested review from JPeer264 and manidlou January 31, 2020 21:17
@RyanZim RyanZim changed the title Update fs promise shims to support latest Node features Update fs promise shims to support latest Node functions Jan 31, 2020
@coveralls
Copy link

coveralls commented Jan 31, 2020

Coverage Status

Coverage decreased (-1.5%) to 81.937% when pulling 8fea403 on ryan/fs-shims into 76e48cb on master.

docs/fs-read-write-writev.md Outdated Show resolved Hide resolved
it('returns an object', () => {
return fs.writev(TEST_FD, TEST_DATA, 0)
.then(({ bytesWritten, buffers }) => {
assert.strictEqual(bytesWritten, SIZE, 'bytesWritten is correct')
Copy link
Collaborator

@manidlou manidlou Feb 1, 2020

Choose a reason for hiding this comment

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

I believe the message should be the opposite, something like bytesWritten is incorrect since that is used as the message for AssertionError if the values are not strictly equal. It is the same for other assertion messages in other tests down below.

https://nodejs.org/api/assert.html#assert_assert_strictequal_actual_expected_message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, perhaps; I don't think it really matters, the main point is to clearly identify which assertion is throwing if you have an error. In any case, these are simply copied from tests earlier in the file, so if we want to fix this, it should be in a separate PR that fixes the entire file.

lib/fs/index.js Outdated Show resolved Hide resolved
@RyanZim RyanZim requested a review from manidlou February 1, 2020 15:02
@RyanZim RyanZim mentioned this pull request Feb 1, 2020
Copy link
Collaborator

@manidlou manidlou left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @RyanZim!

@RyanZim RyanZim merged commit 2e4fcae into master Feb 3, 2020
@RyanZim RyanZim deleted the ryan/fs-shims branch February 3, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants