doc: documentation of sync methods links now to async methods by iwko · Pull Request #21243 · nodejs/node

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

Refs: #21197

vsemozhetbyt

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

Sign in to view

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

This seems unnecessary given the immediately previous sentence.

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

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

@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

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

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

Comment threaddoc/api/fs.md Outdated
This comment was marked as resolved.
Sign in to view
Comment threaddoc/api/fs.md Outdated
This comment was marked as resolved.
Sign in to view

Trott reviewed Jun 10, 2018
View reviewed changes

Comment threaddoc/api/fs.md Outdated
TrottJun 10, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unnecessary given the immediately previous sentence.

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 9921a76Compare 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

Uh oh!

Uh oh!

Conversation

iwko commented Jun 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

TrottJun 10, 2018

Choose a reason for hiding this comment

Uh oh!

TrottJun 10, 2018

Choose a reason for hiding this comment

Uh oh!

Trott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 10, 2018

Uh oh!

vsemozhetbyt commented Jun 10, 2018

Uh oh!

joyeecheung commented Jun 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jun 10, 2018

Uh oh!

iwko commented Jun 11, 2018

Uh oh!

vsemozhetbyt commented Jun 11, 2018

Uh oh!

joyeecheung commented Jun 11, 2018

Uh oh!

vsemozhetbyt commented Jun 17, 2018

Uh oh!

Trott commented Jun 18, 2018

Uh oh!

targos commented Jun 18, 2018

Uh oh!

davisjam commented Jun 19, 2018

Uh oh!

joyeecheung commented Jun 23, 2018

Uh oh!

iwko commented Jun 23, 2018

Uh oh!

iwko commented Jun 23, 2018

Uh oh!

vsemozhetbyt commented Jun 23, 2018

Uh oh!

joyeecheungJun 25, 2018

Choose a reason for hiding this comment

Uh oh!

iwkoJun 25, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheungJun 25, 2018

Choose a reason for hiding this comment

Uh oh!

iwkoJun 28, 2018

Choose a reason for hiding this comment

Uh oh!

iwko commented Jun 29, 2018

Uh oh!

davisjam commented Jun 29, 2018

Uh oh!

vsemozhetbytJun 30, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytJun 30, 2018

Choose a reason for hiding this comment

Uh oh!

davisjam left a comment

Choose a reason for hiding this comment

iwko commented Jun 10, 2018•
edited
Loading

Trott left a comment •
edited
Loading

joyeecheung commented Jun 10, 2018•
edited
Loading

iwkoJun 25, 2018•
edited
Loading