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: unify and compact some fragments in fs.md #20050

Closed
wants to merge 1 commit into from
Closed

doc: unify and compact some fragments in fs.md #20050

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 15, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Refs: #19898 (comment)

@BridgeAR, @ryzokuken , please, let me know if I am missing some other deviations in format patterns.

There is a questionable case in this doc I've abstained to change: many Sync function descriptions contain notes Returns `undefined`. that are not commonly formalized. If I recall correctly, there were attempts to add formal * Returns: `undefined` all-docs-wide, but they were not adopted. In this case, these notes in Sync function descriptions may stress the specificity of these functions, so I've not deleted them during cleaning some other possible tautology. If they should be deleted, let me know.

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 15, 2018
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Apr 15, 2018
@vsemozhetbyt
Copy link
Contributor Author

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM, Will rebase mine if this gets landed first.

@BridgeAR
Copy link
Member

I just looked through the docs briefly and found the following:

stats.isSymbolicLink() (and above seem to have two "returns").

Returns: {boolean}
Returns true if the fs.Stats object describes a symbolic link.

And besides your findings that some functions note that they return undefined a lot of sync functions do not show any return type at all. I think especially in fs it would be useful to add those consistently.

I also noticed that some Returns foo is actually a second sentence and not a new line, e.g., Synchronous version of fs.futimes(). Returns undefined.

Than again, some functions have a bullet point as return type, e.g., fs.writeSync

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2018
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 15, 2018

This is my thoughts on these, but they are not strong opinions, so if there is some minimal consensus to change this I can do it in a new PR:

  1. Sometimes there is a formal return signature and a description for this in next paragraph (if, else, etc ). This is also valid for parameters and options. So maybe all the fs.Stats duplications are just non-formal expanding. But they can be easily merged into above lines,

  2. What would we prefer: delete all the Returns `undefined`. or add them where they are missed and unify them? Considering the second whould be inconsistent with the other docs.

  3. It seems only functions with defined return values have bullet point returns.

@vsemozhetbyt
Copy link
Contributor Author

Landed in 809eb27

@vsemozhetbyt vsemozhetbyt deleted the doc-unify-more-def-ret branch April 16, 2018 11:15
vsemozhetbyt added a commit that referenced this pull request Apr 16, 2018
PR-URL: #20050
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 16, 2018

@nodejs/documentation If anybody has opinions about notes in #20050 (comment), #20050 (comment), and #20050 (comment), please, let us know.

jasnell pushed a commit that referenced this pull request Apr 16, 2018
PR-URL: #20050
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
PR-URL: nodejs#20050
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants