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

doc: add description for filehandle.write(string[, position[, encoding]]) #23224

Closed
wants to merge 1 commit into from

Conversation

darahayes
Copy link

@darahayes darahayes commented Oct 2, 2018

Hey folks, this PR adds missing docs for filehandle.write(string[, position[, encoding]]) in the fs.promises API.

As per #20406, only the buffer version of this API is documented right now.

One thing I wanted to ask. I believe the intention is to reconcile the Promise based API as much as possible with the callback based one. In the docs submitted with this PR, I described the encoding option has having the default value utf8. This is the behaviour in the callback based API.

Based off the code here. I don't think that's actually happening in the Promise based version.

I have two questions:

  • Should the behaviour be modified such that a default value of utf8 is used? I'd be delighted to provide a PR and some tests.
  • Or should I just remove any mention of it from this PR?

Cheers!

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

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Oct 2, 2018
@darahayes darahayes force-pushed the fs.promises.write branch 2 times, most recently from 161f349 to 1350c67 Compare October 2, 2018 18:57
@vsemozhetbyt vsemozhetbyt added the promises Issues and PRs related to ECMAScript promises. label Oct 2, 2018
@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

cc @jasnell

doc/api/fs.md Outdated
the value will be coerced to one.

`position` refers to the offset from the beginning of the file where this data
should be written. If `typeof position !== 'number'` the data will be written at
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be simply written as, "If the type of position is not a number"?

Copy link
Author

Choose a reason for hiding this comment

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

So I basically copied the same wording used here: https://github.com/nodejs/node/pull/23224/files#diff-acabf706a8aa070a8796e3573f7e4678R3910

This style is used in the callback APIs too. I figured I would use the same wording that's used in those other places. I'd be happy to clarify those other places also if you think that's a good idea.

@thefourtheye
Copy link
Contributor

Based off the code here. I don't think that's actually happening in the Promise based version.

Looks like you are correct. If possible, could you come up with a test case which will break because of this?


cc @nodejs/fs

@darahayes
Copy link
Author

If possible, could you come up with a test case which will break because of this?

Sure thing. I'll look to see how it's tested in the callback based API and see if I can get some inspiration from there.

@benjamingr
Copy link
Member

Should the behaviour be modified such that a default value of utf8 is used? I'd be delighted to provide a PR and some tests.

I believe so - yes, and work improving the promises fs API is always appreciated. (cc @jasnell )

Changes in this PR LGTM

`encoding` is the expected string encoding.

It is unsafe to use `filehandle.write()` multiple times on the same file
without waiting for the `Promise` to be resolved (or rejected). For this
Copy link
Member

Choose a reason for hiding this comment

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

Nit: a rejected promise is still resolved (and so is a promise following another promise) - fulfilled instead of resolved or "settled" instead of either would work here.

Copy link
Author

Choose a reason for hiding this comment

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

Hey there, I've addressed some other feedback but I haven't addressed this one yet. I did that for two reasons.

  1. This is the same wording used in other parts of the documentation. See the docs for the filehandle.write(buffer)
  2. I would argue that explicitly using the words resolved and rejected make sense in this context. If you're familiar with Promises, this makes sense. I would argue the same cannot exactly be said about the word fulfilled because it's not as commonly used when referring to Promises. (at least as far as I can tell). In my opinion that potentially introduces some ambiguity.

All that being said: I am happy to make the suggested changes here, as well as in the other parts of the same doc to ensure consistency.

@darahayes
Copy link
Author

Hey folks, sorry I was away for the past couple of days so I couldn't do any more work on this. I mentioned that I would try to come up with some test cases wrt to the encoding option. Still willing to do this, but I'm wondering should it be done as part of this PR or as a different one? Thanks!

@thefourtheye
Copy link
Contributor

@darahayes I would say, you can add them as separate commits in this PR itself. Because without the code changes, this doc patch would not be correct.

doc/api/fs.md Show resolved Hide resolved
@darahayes
Copy link
Author

Hey folks, it's been so long but I was finally able to get some more time with this issue.

I've added a code change that ensures the same behaviour as the callback-based API. A default value of 'utf8' is used for the encoding when none is supplied.

All that being said, there was a request to come up with a test case for this and I'm struggling with that a little bit.

The callback based fs.write(filehandle, string) method only has a single test and it doesn't test the behaviour around defaulting to utf8.

I'm not 100% sure how to test that the string contents written to a file is indeed utf8 encoded. Anyone have any ideas? Thanks

@darahayes
Copy link
Author

@thefourtheye Hey there. Sorry to bother you like this, I figured I would ping you as the original reviewer. I know you folks were probably really busy with all the PRs from the recent code and learn events 😃. Just wanted to bump for some visibility.

Just a quick summary of where I'm at - I believe I have made the necessary changes but I'm not 100% sure how write a good test case for this. I was hoping I could get some pointers if anyone has the time. Thanks!

@thefourtheye
Copy link
Contributor

@darahayes No problem at all. I am sorry I couldn't get to this on time. Let me take a look at this today. Thanks for being patient.

@thefourtheye
Copy link
Contributor

Okay, looks like I was wrong.

In the docs submitted with this PR, I described the encoding option has having the default value utf8. This is the behaviour in the callback based API.

Based off the code here. I don't think that's actually happening in the Promise based version.

I took a deeper look at the code and the default encoding value is actually set to UTF-8. Let me break this down.

const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
length, kUsePromises)) || 0;

which actually parses the encoding like this

const auto enc = ParseEncoding(isolate, args[3], UTF8);

args[3] is actually the length. So, ParseEncoding does this

node/src/node_encoding.cc

Lines 89 to 100 in fcd7a72

enum encoding ParseEncoding(Isolate* isolate,
Local<Value> encoding_v,
enum encoding default_encoding) {
CHECK(!encoding_v.IsEmpty());
if (!encoding_v->IsString())
return default_encoding;
Utf8Value encoding(isolate, encoding_v);
return ParseEncoding(*encoding, default_encoding);
}

If the length parameter is actually not a String then the default encoding (UTF8) will be used. Also, if the passed length value is not a valid encoding type, then

} else if (StringEqualNoCase(encoding, "buffer")) {
return BUFFER;
} else if (StringEqualNoCase(encoding, "hex")) {
return HEX;
} else {
return default_encoding;
}

will take care of it.


@darahayes Sorry, I misled you. Could you please drop the commit which introduces default value for encoding?

Add missing docs for filehandle.write(string[, position[, encoding]])
In the fs.promises API.

fixes: nodejs#20406
@darahayes
Copy link
Author

@thefourtheye thanks so much for your help! I'm kicking myself that I didn't figure that out sooner. I've dropped the commit you mentioned and I also addressed some of the feedback on the docs changes.

@thefourtheye
Copy link
Contributor

thefourtheye commented Nov 14, 2018

@thefourtheye
Copy link
Contributor

@nodejs/fs PTAL once.

@Trott
Copy link
Member

Trott commented Nov 20, 2018

Landed in 692b09f

@Trott Trott closed this Nov 20, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 20, 2018
Add missing docs for filehandle.write(string[, position[, encoding]])
In the fs.promises API.

Fixes: nodejs#20406
PR-URL: nodejs#23224
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@thefourtheye
Copy link
Contributor

Thanks @Trott. I am currently traveling, otherwise I would have landed this sooner.

@darahayes Thanks for your patience in getting this done.

targos pushed a commit that referenced this pull request Nov 20, 2018
Add missing docs for filehandle.write(string[, position[, encoding]])
In the fs.promises API.

Fixes: #20406
PR-URL: #23224
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Add missing docs for filehandle.write(string[, position[, encoding]])
In the fs.promises API.

Fixes: #20406
PR-URL: #23224
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
codebytere pushed a commit that referenced this pull request Jan 13, 2019
Add missing docs for filehandle.write(string[, position[, encoding]])
In the fs.promises API.

Fixes: #20406
PR-URL: #23224
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Add missing docs for filehandle.write(string[, position[, encoding]])
In the fs.promises API.

Fixes: nodejs#20406
PR-URL: nodejs#23224
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@codebytere codebytere mentioned this pull request Jan 15, 2019
codebytere pushed a commit that referenced this pull request Jan 29, 2019
Add missing docs for filehandle.write(string[, position[, encoding]])
In the fs.promises API.

Fixes: #20406
PR-URL: #23224
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants