doc: documentation of sync methods links now to async methods #21243

iwko · 2018-06-10T13:36:31Z

Documentation of sync methods links now to async methods if it made sense

Checklist

documentation is changed or added
commit message follows commit guidelines

vsemozhetbyt

Thank you! There are some nits we need to fix)

doc/api/fs.md

@@ -1826,6 +1830,8 @@ added: v0.8.6

 Synchronous ftruncate(2). Returns `undefined`.

+For detailed information, see the documentation of [`fs.ftruncate()`][].


doc/api/fs.md

@@ -2867,6 +2883,8 @@ changes:

 Synchronous symlink(2). Returns `undefined`.

+For detailed information, see the documentation of [`fs.symlink()`][].


Trott · 2018-06-10T15:53:28Z

doc/api/fs.md

@@ -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 seems unnecessary given the immediately previous sentence.

Trott · 2018-06-10T15:54:36Z

doc/api/fs.md

@@ -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()`][].


I would greatly prefer we stick with the wording used elsewhere:

This is the synchronous version of [`fs.exists()`][].

Trott

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.

Trott · 2018-06-10T15:58:19Z

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!

vsemozhetbyt · 2018-06-10T15:58:31Z

cc @joyeecheung to chime in.

joyeecheung · 2018-06-10T19:35:53Z

@Trott See #21197 and #21193

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 Synchronous version of <a link> is not enough IMO, people would just think that the documentation of the async API would not be informative either if they don't get curious and actually click the link.

Maybe a better way to fix this would be instead of saying Synchronous version of <a link>, say

For detailed information, see the documentation of the asynchronous
version of this API: <a link>

But I guess the point of saying Synchronous version of is to make sure people are aware that the async version is preferred?

Trott · 2018-06-10T21:17:58Z

For detailed information, see the documentation of the asynchronous version of this API:

@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.

@nodejs/documentation

reconsidering

iwko · 2018-06-11T16:53:04Z

So, should I change my changes to For detailed information, see the documentation of the asynchronous version of this API: <a link>?

vsemozhetbyt · 2018-06-11T16:53:37Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/874/

joyeecheung · 2018-06-11T17:29:12Z

So, should I change my changes to For detailed information, see the documentation of the asynchronous version of this API: ?

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.

vsemozhetbyt · 2018-06-17T08:22:42Z

How should we proceed?

Trott · 2018-06-18T04:04:15Z

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...

targos · 2018-06-18T04:42:12Z

The text LGTM as is

davisjam · 2018-06-19T00:37:11Z

@iwko Thank you for this PR! Here are some comments.

I share @Trott's concerns about repetition. While it's true that "This is the synchronous version of X" doesn't necessarily indicate that X has more complete documentation, I'm not sure that presenting this in separate sentences like "This is the synchronous version of X. See X for the full API." is the right approach.

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: a link". I don't think we need to explicitly mention everywhere that you should use asynchronous APIs. That's more the job of guides like this one. I don't think API docs need to take as firm a stance on this, especially not on a per-API basis.

fs is not the only module with synchronous and asynchronous versions. For example, the crypto module has synchronous and asynchronous versions of pbkdf2. The approach to documenting the sync vs. async versions seems inconsistent between fs and crypto. Should this PR attempt to achieve uniformity across all such modules?

joyeecheung · 2018-06-23T05:15:24Z

@davisjam I agree that merging the sentences would more better, but we can probably leave the crypto docs for another PR.

@iwko Can you update the PR according to the suggestions above? Thanks!

iwko · 2018-06-23T07:32:02Z

@joyeecheung Sure thing

iwko · 2018-06-23T08:28:54Z

@joyeecheung done

vsemozhetbyt · 2018-06-23T14:54:17Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/890/

joyeecheung · 2018-06-25T00:17:23Z

doc/api/fs.md

@@ -1143,6 +1143,9 @@ changes:
 Synchronously changes the permissions of a file. Returns `undefined`.
 This is the synchronous version of [`fs.chmod()`][].


@iwko I believe @davisjam and @Trott and I meant that sentence like

This is the synchronous version of [`fs.chmod()`][].

Should be removed after sentences like

For detailed information, see the documentation of the asynchronous version of this API: [`fs.chmod()`][].

are added to avoid duplication?

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).
?

@iwko Yes, that's my understanding

@joyeecheung done

iwko · 2018-06-29T12:09:56Z

I've done all changes. What do You think? @vsemozhetbyt @Trott @davisjam @joyeecheung

davisjam · 2018-06-29T16:17:49Z

@iwko I will have time to look today or tomorrow.

vsemozhetbyt · 2018-06-30T12:04:22Z

doc/api/fs.md

@@ -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`.


Is it worth to completely skip these notes? We usually do not state anything if a function returns undefined.

vsemozhetbyt · 2018-06-30T12:09:40Z

doc/api/fs.md

@@ -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


Nit: this one should go after the next reference, ABC-wise.

davisjam

LGTM, thank you!

iwko · 2018-07-08T19:25:57Z

When this PR will be merged?

Trott · 2018-07-08T19:45:46Z

When this PR will be merged?

@iwko Can you address the two nits from @vsemozhetbyt? Specifically, there's no need to indicate that undefined is returned by a function, and there's one reference that is out of alphabetical order.

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.)

iwko · 2018-07-08T20:45:32Z

@Trott I've made changes

vsemozhetbyt · 2018-07-08T21:19:55Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/905/

fs.md

Trott · 2018-07-09T04:28:43Z

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.

iwko · 2018-07-09T07:30:39Z

@vsemozhetbyt @joyeecheung @davisjam @TimothyGu @jasnell

Are you still ok with those changes?

vsemozhetbyt · 2018-07-09T07:42:32Z

Still LGTM)

mcollina

LGTM

Add links to async methods and make wording consistent. PR-URL: nodejs#21243 Refs: nodejs#21197 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Trott · 2018-07-09T21:46:09Z

Landed in 65208d0.

Thanks for the contribution! 🎉

Add links to async methods and make wording consistent. PR-URL: #21243 Refs: #21197 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jun 10, 2018

vsemozhetbyt reviewed Jun 10, 2018

View reviewed changes

Trott reviewed Jun 10, 2018

View reviewed changes

Trott previously requested changes Jun 10, 2018

View reviewed changes

TimothyGu mentioned this pull request Jun 23, 2018

doc: explicitly mention that the explanation of an synchronous fs API live in the docs of the async API #21197

Closed

BeniCheni mentioned this pull request Jun 23, 2018

doc: explicitly mention sync APIs with doc links of the related async… #21479

Closed

3 tasks

TimothyGu approved these changes Jun 24, 2018

View reviewed changes

joyeecheung reviewed Jun 25, 2018

View reviewed changes

joyeecheung approved these changes Jun 29, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 29, 2018

jasnell approved these changes Jun 29, 2018

View reviewed changes

vsemozhetbyt approved these changes Jun 30, 2018

View reviewed changes

davisjam approved these changes Jun 30, 2018

View reviewed changes

iwko added 7 commits July 8, 2018 21:25

doc: documentation of sync methods links now to async methods

bc0d71c

doc: Link fs.ftruncate and fs.symlink methods

651d11a

doc: fs sync methods links now to async methods

1dc060a

doc: fix max line length in fs.md

373e6fd

doc: Remove sentences like 'This is the synchronous version of..' from

c53f7f4

fs.md

doc: change fs.chmod() description

36a392e

doc: remove unnecessary info about returned value in fs.chmod.

9921a76

Trott force-pushed the fs-doc-sync-methods branch from 038be2d to 9921a76 Compare July 9, 2018 04:26

mcollina approved these changes Jul 9, 2018

View reviewed changes

Trott closed this Jul 9, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

		@@ -1826,6 +1830,8 @@ added: v0.8.6

		Synchronous ftruncate(2). Returns `undefined`.

		For detailed information, see the documentation of [`fs.ftruncate()`][].

		@@ -2867,6 +2883,8 @@ changes:

		Synchronous symlink(2). Returns `undefined`.

		For detailed information, see the documentation of [`fs.symlink()`][].

		@@ -1143,6 +1143,9 @@ changes:
		Synchronously changes the permissions of a file. Returns `undefined`.
		This is the synchronous version of [`fs.chmod()`][].

doc: documentation of sync methods links now to async methods #21243

doc: documentation of sync methods links now to async methods #21243

Conversation

iwko commented Jun 10, 2018 • edited Loading

Checklist

vsemozhetbyt left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Trott Jun 10, 2018

Choose a reason for hiding this comment

Trott Jun 10, 2018

Choose a reason for hiding this comment

Trott left a comment • edited Loading

Choose a reason for hiding this comment

Trott commented Jun 10, 2018

vsemozhetbyt commented Jun 10, 2018

joyeecheung commented Jun 10, 2018 • edited Loading

Trott commented Jun 10, 2018

iwko commented Jun 11, 2018

vsemozhetbyt commented Jun 11, 2018

joyeecheung commented Jun 11, 2018

vsemozhetbyt commented Jun 17, 2018

Trott commented Jun 18, 2018

targos commented Jun 18, 2018

davisjam commented Jun 19, 2018

joyeecheung commented Jun 23, 2018

iwko commented Jun 23, 2018

iwko commented Jun 23, 2018

vsemozhetbyt commented Jun 23, 2018

joyeecheung Jun 25, 2018

Choose a reason for hiding this comment

iwko Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Jun 25, 2018

Choose a reason for hiding this comment

iwko Jun 28, 2018

Choose a reason for hiding this comment

iwko commented Jun 29, 2018

davisjam commented Jun 29, 2018

vsemozhetbyt Jun 30, 2018

Choose a reason for hiding this comment

vsemozhetbyt Jun 30, 2018

Choose a reason for hiding this comment

davisjam left a comment

Choose a reason for hiding this comment

iwko commented Jul 8, 2018

Trott commented Jul 8, 2018

iwko commented Jul 8, 2018

vsemozhetbyt commented Jul 8, 2018

Trott commented Jul 9, 2018

iwko commented Jul 9, 2018

vsemozhetbyt commented Jul 9, 2018

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Jul 9, 2018

iwko commented Jun 10, 2018 •

edited

Loading

Trott left a comment •

edited

Loading

joyeecheung commented Jun 10, 2018 •

edited

Loading

iwko Jun 25, 2018 •

edited

Loading