doc: add flags section to document all flags by indranil · Pull Request #20042 · nodejs/node

indranil · 2018-04-15T02:02:20Z

Adds a section at the very end to document all flags and links to it from every function that takes a flag argument.

I kept the flags that were in fs.open() for people who are familiar with the docs already.

Checklist

documentation is changed or added
commit message follows commit guidelines

trivikr · 2018-04-15T03:02:55Z

It'll be helpful if flags are sorted in alphabetical order

ok, I'm on it!
Would it also make sense to rearrange the existing flag on open() to also be rearranged, or is there a reason to keep the other ones in their original order?

I'll let @nodejs/fs take a call, in fs.open() the flags are separated into read and write ones.
I don't think the description for flags should be repeated in the documentation. In my opinion, fs.open() should also link to this description for flags.

I think sorting the flags is a good idea.

Thanks, they should now be sorted and consolidated in one area!

joyeecheung

Can you replace the description of flags in fs.open with a link to the newly added section to avoid duplication?

indranil · 2018-04-15T09:23:27Z

Removed any other area where the flag descriptions were being duplicated into one area that is being linked from everywhere.

trivikr · 2018-04-15T16:06:22Z

The behavior of fs.open() is platform-specific for some flags

In fsPromises.open(), a FileHandle instead of file descriptor is returned. Are those different @joyeecheung ?

Good point!
I thought that both fs.open() and fsPromises.open() have the same caveats, so I included those together. If FileHandle differs from the file descriptor, then it might make sense to put them within their respective function descriptions.

trivikr · 2018-04-15T16:13:27Z

File System flags is not part of FS Constants, this should be a separate section by changing to ## File System Flags

BridgeAR · 2018-04-15T20:34:07Z

Nit: please always just use single line breaks. So all the added line breaks here are obsolete.

Thank you for the pointer! Fixed in latest commit!

BridgeAR · 2018-04-15T20:34:39Z

@nodejs/documentation @nodejs/fs PTAL

vsemozhetbyt · 2018-04-15T22:26:02Z

It seems we have two full sentences here now, so we need to add periods after the both to be consistent with other docs:
See [support of file system `flags`][]. **Default:** `'a'`.
The same for all the cases below (if Default is missing then only one period is needed).

Resolved in commit 14da387

vsemozhetbyt · 2018-04-15T22:34:57Z

There are only 2 references here out of ASCII sorting, so let's do not increase the disorder and add this at the end of the list, after [inode]: and two other stray references.

Resolved in 14da387

indranil · 2018-04-16T06:49:12Z

Sign in to view

This should be taken care of as well :)

vsemozhetbyt · 2018-04-15T23:31:45Z

a file descriptor -> a file descriptor or a `FileHandle`?

Resolved in 14da387

vsemozhetbyt · 2018-04-15T23:34:26Z

`path` -> the path?

All path -> the path resolved in 14da387

vsemozhetbyt · 2018-04-15T23:34:34Z

`path` -> the path?

vsemozhetbyt · 2018-04-15T23:40:38Z

`path` -> the path?

vsemozhetbyt · 2018-04-15T23:40:47Z

`path` -> the path?

vsemozhetbyt · 2018-04-15T23:52:59Z

`fs.open` or `fs.writeFile` -> `fs.open()` or `fs.writeFile()`?

vsemozhetbyt

It seems nothing is missed while merging.
LGTM with nits addressed.
Thank you!

trivikr · 2018-04-18T06:36:02Z

@indranil Kindly remove merge commit and use rebase instead as described in contributing guidelines

indranil · 2018-04-20T04:24:04Z

sorry this took so long, but finally removed and rebased and repushed!

trivikr · 2018-04-20T05:42:34Z

@indranil The nits mentioned by @vsemozhetbyt still need to be addressed.

indranil · 2018-04-20T09:37:43Z

omg so sorry my merge undid all those changes! redid them and pushed them, thank you!

vsemozhetbyt · 2018-04-20T09:49:57Z

Md-lint finds trailing tab here)

Resolved in 4b70819

vsemozhetbyt · 2018-04-20T09:50:34Z

Md-lint finds trailing tab here)

Adds a section at the very end to document all flags and links to it from every function that takes a flag argument. Fixes: nodejs#16971

Removed description of flags into one place, linking to it from all other references. Also rearranged the flags alphabetically, keeping connected ones together. Fixes: nodejs#16971

Addressing comments

Removed unnecessary newlines from the end of document

Finally able to resolve the merge conflicts

All changes in the previous commits, consolidated location, adding periods, putting notes together were wiped away by my clumsy merge. Redid them all!

Two trailing tabs caught by the linter removed

indranil · 2018-04-20T11:19:53Z

thank you for that! fixed that now :)

vsemozhetbyt · 2018-04-20T13:00:08Z

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

vsemozhetbyt · 2018-04-20T13:10:27Z

If there are no objections, I will land soon.

trivikr · 2018-04-20T15:44:47Z

Landed in 53822f6

Adds a section at the very end to document all flags and links to it from every function that takes a flag argument. Fixes: #16971 PR-URL: #20042 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@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 Apr 15, 2018

trivikr requested a review from joyeecheung April 15, 2018 02:55

trivikr reviewed Apr 15, 2018
View reviewed changes

joyeecheung reviewed Apr 15, 2018
View reviewed changes

trivikr reviewed Apr 15, 2018
View reviewed changes

trivikr mentioned this pull request Apr 15, 2018
doc: add section in fs to document mode argument #20049
Closed

indranil force-pushed the docs-fs-extract-flags-into-own-section branch from c5a7c21 to c73d576Compare April 15, 2018 16:40

BridgeAR reviewed Apr 15, 2018
View reviewed changes

indranil force-pushed the docs-fs-extract-flags-into-own-section branch from c73d576 to 5134ffcCompare April 15, 2018 20:57

vsemozhetbyt reviewed Apr 15, 2018
View reviewed changes

Comment threaddoc/api/fs.md Outdated
vsemozhetbytApr 15, 2018•
edited
Loading
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`path` -> the path?

vsemozhetbyt reviewed Apr 15, 2018
View reviewed changes

Comment threaddoc/api/fs.md Outdated
vsemozhetbytApr 15, 2018•
edited
Loading
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`path` -> the path?

vsemozhetbyt reviewed Apr 15, 2018
View reviewed changes

Comment threaddoc/api/fs.md Outdated
vsemozhetbytApr 15, 2018•
edited
Loading
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`path` -> the path?

vsemozhetbyt reviewed Apr 15, 2018
View reviewed changes

Comment threaddoc/api/fs.md Outdated
vsemozhetbytApr 15, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`fs.open` or `fs.writeFile` -> `fs.open()` or `fs.writeFile()`?

vsemozhetbyt approved these changes Apr 15, 2018
View reviewed changes

indranil force-pushed the docs-fs-extract-flags-into-own-section branch from 5134ffc to 9ee3214Compare April 16, 2018 06:47

This comment has been minimized.
Sign in to view

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

trivikr approved these changes Apr 16, 2018
View reviewed changes

trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 18, 2018

indranil force-pushed the docs-fs-extract-flags-into-own-section branch from 26bdc14 to c57b08cCompare April 20, 2018 04:23

This comment has been minimized.
Sign in to view

trivikr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 20, 2018

indranil force-pushed the docs-fs-extract-flags-into-own-section branch from c57b08c to f51eef1Compare April 20, 2018 09:35

This comment has been minimized.
Sign in to view

vsemozhetbyt reviewed Apr 20, 2018
View reviewed changes

Comment threaddoc/api/fs.md Outdated
vsemozhetbytApr 20, 2018
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Md-lint finds trailing tab here)

indranil added 7 commits April 20, 2018 13:17

doc: add flags section to document all flags
1588796
Adds a section at the very end to document all flags and links to it from every function that takes a flag argument. Fixes: nodejs#16971

doc: consolidated flag options in one central place and rearranged list
241b7cd
Removed description of flags into one place, linking to it from all other references. Also rearranged the flags alphabetically, keeping connected ones together. Fixes: nodejs#16971

doc: create file system flags its own section
824d15e
Addressing comments

doc: removed extra newlines
090a272
Removed unnecessary newlines from the end of document

doc: resolving merge conflicts
a7af43c
Finally able to resolve the merge conflicts

doc: re-added all changes wiped away by merge
14da387
All changes in the previous commits, consolidated location, adding periods, putting notes together were wiped away by my clumsy merge. Redid them all!

doc: removed two trailing tabs
4b70819
Two trailing tabs caught by the linter removed

indranil force-pushed the docs-fs-extract-flags-into-own-section branch from f51eef1 to 4b70819Compare April 20, 2018 11:18

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

trivikr approved these changes Apr 20, 2018
View reviewed changes

trivikr closed this Apr 20, 2018

Uh oh!

Uh oh!

Conversation

indranil commented Apr 15, 2018

Checklist

Uh oh!

trivikrApr 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

indranil commented Apr 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 15, 2018

Uh oh!

vsemozhetbytApr 15, 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!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytApr 15, 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!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytApr 15, 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!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytApr 15, 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!

vsemozhetbytApr 15, 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!

vsemozhetbytApr 15, 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!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

trivikrApr 15, 2018•
edited
Loading

vsemozhetbytApr 15, 2018•
edited
Loading

vsemozhetbytApr 15, 2018•
edited
Loading

vsemozhetbytApr 15, 2018•
edited
Loading

vsemozhetbytApr 15, 2018•
edited
Loading

vsemozhetbytApr 15, 2018•
edited
Loading

vsemozhetbytApr 15, 2018•
edited
Loading

vsemozhetbytApr 20, 2018•
edited
Loading