Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign updoc: documentation of sync methods links now to async methods #21243
Conversation
nodejs-github-bot
added
doc
fs
labels
Jun 10, 2018
vsemozhetbyt
reviewed
Jun 10, 2018
|
Thank you! There are some nits we need to fix) |
| @@ -1826,6 +1830,8 @@ added: v0.8.6 | |||
|
|
|||
| Synchronous ftruncate(2). Returns `undefined`. | |||
|
|
|||
| For detailed information, see the documentation of [`fs.ftruncate()`][]. | |||
This comment was marked as resolved.
This comment was marked as resolved.
vsemozhetbyt
Jun 10, 2018
Member
It seems we miss the bottom reference for this link. You can add this to the bottom list (in ASCII sorting order):
[`fs.ftruncate()`]: #fs_fs_ftruncate_fd_len_callback| @@ -2867,6 +2883,8 @@ changes: | |||
|
|
|||
| Synchronous symlink(2). Returns `undefined`. | |||
|
|
|||
| For detailed information, see the documentation of [`fs.symlink()`][]. | |||
This comment was marked as resolved.
This comment was marked as resolved.
vsemozhetbyt
Jun 10, 2018
Member
It seems we miss the bottom reference for this link. You can add this to the bottom list (in ASCII sorting order):
[`fs.symlink()`]: #fs_fs_symlink_target_path_type_callback
Trott
reviewed
Jun 10, 2018
| @@ -1143,6 +1143,8 @@ changes: | |||
| Synchronously changes the permissions of a file. Returns `undefined`. | |||
| This is the synchronous version of [`fs.chmod()`][]. | |||
|
|
|||
| For detailed information, see the documentation of [`fs.chmod()`][]. | |||
This comment has been minimized.
This comment has been minimized.
Trott
reviewed
Jun 10, 2018
| @@ -1573,6 +1575,8 @@ changes: | |||
| Synchronous version of [`fs.exists()`][]. | |||
| Returns `true` if the path exists, `false` otherwise. | |||
|
|
|||
| For detailed information, see the documentation of [`fs.exists()`][]. | |||
This comment has been minimized.
This comment has been minimized.
Trott
Jun 10, 2018
Member
I would greatly prefer we stick with the wording used elsewhere:
This is the synchronous version of [`fs.exists()`][].
Trott
reviewed
Jun 10, 2018
|
Many of these contain links we should definitely add while many others of these changes also seem to add unnecessary sentences, as they often repeat the information in the sentence that comes right before them. |
This comment has been minimized.
This comment has been minimized.
|
Hi, @iwko! Welcome, and thanks for the pull request! Documentation changes can often attract a lot of review comments. I hope you'll be patient and not be discouraged. Thanks! |
This comment has been minimized.
This comment has been minimized.
|
cc @joyeecheung to chime in. |
This comment has been minimized.
This comment has been minimized.
|
Also I was confused when I got started with fs documentation as well. There are no clear indications that you should click the link to find out more information e.g. about what the arguments actually mean. Just listing arguments without any explanation following by Maybe a better way to fix this would be instead of saying
But I guess the point of saying |
This comment has been minimized.
This comment has been minimized.
@joyeecheung Something like that would make sense to me. But given what you wrote, I guess the current version of this is still an improvement. I'll remove my request for changes. This can always be iterated on going forward. |
Trott
dismissed
their
stale review
Jun 10, 2018
reconsidering
This comment has been minimized.
This comment has been minimized.
|
So, should I change my changes to |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I would say yes, but let's see what other people from @nodejs/fs @nodejs/documentation think. With that style we will lose the emphasis on making the async versions the perferred APIs. |
This comment has been minimized.
This comment has been minimized.
|
How should we proceed? |
This comment has been minimized.
This comment has been minimized.
|
I hate to do this but this is more than a week old and has attracted very little attention from @nodejs/documentation so I'm going to do the dreaded @nodejs/collaborators... |
This comment has been minimized.
This comment has been minimized.
|
The text LGTM as is |
This comment has been minimized.
This comment has been minimized.
|
@iwko Thank you for this PR! Here are some comments.
We could prefix this document with "The asynchronous version of APIs documents parameters. The synchronous version indicates only the return value and exceptional behavior.". That would only help someone who bothers to read the complete document though. I would prefer merging the two into a single sentence as @Trott proposed: "For detailed information, see the documentation of the asynchronous version of this API:
|
TimothyGu
referenced this pull request
Jun 23, 2018
Closed
doc: explicitly mention that the explanation of an synchronous fs API live in the docs of the async API #21197
BeniCheni
referenced this pull request
Jun 23, 2018
Closed
doc: explicitly mention sync APIs with doc links of the related async… #21479
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@joyeecheung Sure thing |
This comment has been minimized.
This comment has been minimized.
|
@joyeecheung done |
This comment has been minimized.
This comment has been minimized.
TimothyGu
approved these changes
Jun 24, 2018
joyeecheung
reviewed
Jun 25, 2018
| @@ -1143,6 +1143,9 @@ changes: | |||
| Synchronously changes the permissions of a file. Returns `undefined`. | |||
| This is the synchronous version of [`fs.chmod()`][]. | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
iwko
Jun 25, 2018
•
Author
Contributor
I am a little confused now. So documentation for fs.chmod() should look like this?
Synchronously changes the permissions of a file. Returns undefined. For detailed information, see the documentation of the asynchronous version of this API: [fs.chmod()][]. See also: chmod(2).
?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I've done all changes. What do You think? @vsemozhetbyt @Trott @davisjam @joyeecheung |
joyeecheung
approved these changes
Jun 29, 2018
joyeecheung
added
the
author ready
label
Jun 29, 2018
This comment has been minimized.
This comment has been minimized.
|
@iwko I will have time to look today or tomorrow. |
jasnell
approved these changes
Jun 29, 2018
vsemozhetbyt
approved these changes
Jun 30, 2018
| @@ -1140,8 +1140,10 @@ changes: | |||
| * `path` {string|Buffer|URL} | |||
| * `mode` {integer} | |||
|
|
|||
| Synchronously changes the permissions of a file. Returns `undefined`. | |||
| This is the synchronous version of [`fs.chmod()`][]. | |||
| Returns `undefined`. | |||
This comment has been minimized.
This comment has been minimized.
vsemozhetbyt
Jun 30, 2018
Member
Is it worth to completely skip these notes? We usually do not state anything if a function returns undefined.
| @@ -4589,6 +4621,7 @@ the file contents. | |||
| [`fs.chown()`]: #fs_fs_chown_path_uid_gid_callback | |||
| [`fs.copyFile()`]: #fs_fs_copyfile_src_dest_flags_callback | |||
| [`fs.exists()`]: fs.html#fs_fs_exists_path_callback | |||
| [`fs.ftruncate()`]: #fs_fs_ftruncate_fd_len_callback | |||
This comment has been minimized.
This comment has been minimized.
davisjam
approved these changes
Jun 30, 2018
|
LGTM, thank you! |
This comment has been minimized.
This comment has been minimized.
|
When this PR will be merged? |
This comment has been minimized.
This comment has been minimized.
@iwko Can you address the two nits from @vsemozhetbyt? Specifically, there's no need to indicate that Once that's done, we can run CI and probably land this promptly. (There may be an issue with the merge commit recently added to the branch. Our CI setup does not like merge commits, or at least didn't used to. But we'll deal with that when we get there.) |
This comment has been minimized.
This comment has been minimized.
|
@Trott I've made changes |
This comment has been minimized.
This comment has been minimized.
iwko
added some commits
Jun 10, 2018
Trott
force-pushed the
iwko:fs-doc-sync-methods
branch
from
038be2d
to
9921a76
Jul 9, 2018
This comment has been minimized.
This comment has been minimized.
|
Looks like CI failed because of the merge commit. I've squashed out the merge commit. CI: https://ci.nodejs.org/job/node-test-pull-request-lite/906/ Would probably be good for people who have approved this to look it over again and make sure they still approve. I don't expect anyone to rescind their approval but this has nonetheless gone through substantial evolution. |
This comment has been minimized.
This comment has been minimized.
|
@vsemozhetbyt @joyeecheung @davisjam @TimothyGu @jasnell Are you still ok with those changes? |
This comment has been minimized.
This comment has been minimized.
|
Still LGTM) |
Trott
added a commit
to Trott/io.js
that referenced
this pull request
Jul 9, 2018
This comment has been minimized.
This comment has been minimized.
|
Landed in 65208d0. Thanks for the contribution! |
iwko commentedJun 10, 2018
•
edited
Documentation of sync methods links now to async methods if it made sense
Checklist
Refs: #21197