doc: adds clarification for fs constants for node < v6.3.x in fs.md by anshulguleria · Pull Request #12690 · nodejs/node

anshulguleria · 2017-04-27T07:23:14Z

Added NOTE in fs.access documentation to explain the position of
constants(R_OK, etc) in fs for node versions < v6.3.x
Added clarification on FS Constants documentation for the same.

Refs: Issue #8044

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

vsemozhetbyt · 2017-04-27T07:37:13Z

doc/api/fs.md

Thank you for the contribution!

A nit: could you please wrap the line <= 80 characters according to style guide?

Also, see commit guidelines: commit title should be <= 50 characters and use imperative verb (add) after the subsystem tag.

All this can be fixed by a collaborator who will land the PR though)

anshulguleria · 2017-04-27T09:00:55Z

@vsemozhetbyt Thanks for review, I will take care of line lengths but how should I take care of commit message? Do I need to commit again raising new pull request? Because I don't know how I can change commit message of already pushed changes?

vsemozhetbyt · 2017-04-27T09:06:14Z

@anshulguleria You can use git commit --amend and git push --force-with-lease. However, if you are not sure for now how to amend commits, it is OK, this can be addressed in landing time by another collaborator)

anshulguleria · 2017-04-27T10:24:26Z

@vsemozhetbyt Done. Thanks for help.

cjihrig

Is it possible to utilize the individual changelogs for this? It seems like the perfect use case.

jasnell · 2017-04-27T16:13:27Z

doc/api/fs.md

Please avoid the use of you in the doc text. :-) ...

Removed you from doc.

jasnell · 2017-04-27T16:13:48Z

doc/api/fs.md

s/node/Node.js

Do you want to change node to Node?

anshulguleria · 2017-04-28T09:40:54Z

@cjihrig I saw the changelogs of v6.3.0 here but couldn't find any change regarding this. But when I test this on Node v6.3.0 I can see that fs.constants exist there.

cjihrig · 2017-04-28T12:51:49Z

@anshulguleria I meant the individual changelogs in the API docs. For example, check out the <!-- YAML markup on fs.appendFile() in doc/api/fs.md.

anshulguleria · 2017-05-02T05:24:16Z

Hi @cjihrig I saw the <!-- YAML changlogs for fs.appendFile() but that markup doesn't shows up in documentation, when I see it here. So even if we add this information there it wouldn't help IMHO.

anshulguleria · 2017-05-02T05:35:21Z

Hi @jasnell I have done the changes requested by you. Have a look.

cjihrig · 2017-05-02T13:18:48Z

Check the v7 branch - https://nodejs.org/dist/latest-v7.x/docs/api/fs.html#fs_fs_appendfile_file_data_options_callback. Specifically, look at the "History" with the little arrow next to it.

anshulguleria · 2017-05-03T13:31:12Z

@cjihrig Can you help me setup nodedocs locally? I am not able to find how to do it on my local machine. Because if I run those .md files through some other local markdown processor like harp.js those <!-- YAML tags dont gets rendered.

anshulguleria · 2017-05-03T15:20:40Z

@cjihrig Updated doc to use changelog instead of Note.

addaleax · 2017-05-03T19:39:12Z

Can you help me setup nodedocs locally? I

@anshulguleria Run make -j4 doc-only in your node checkout, that will create the HTML files in out/doc/api/ :)

anshulguleria · 2017-05-06T05:07:05Z

I have done the requested changes. Do I need to do something else also to get them accepted? Sorry if I am missing something basic in this process, I am new to this.

jasnell · 2017-05-06T05:11:24Z

At this point we just need to finish the review and get sign off. Quite a few of us have been busy at the collaborator summit meeting in Berlin so we haven't been as responsive as usual. I'm sure this will get a good look in the next day or two. If all looks good, it should land in a couple of days.

gibfahn

This mostly LGTM, left a few nits.

gibfahn · 2017-05-06T09:40:07Z

doc/api/fs.md

as soft a -> as a soft

gibfahn · 2017-05-06T09:40:47Z

doc/api/fs.md

fs.constatns -> fs.constants

gibfahn · 2017-05-06T09:41:05Z

doc/api/fs.md

. Or -> , or

gibfahn · 2017-05-06T09:42:39Z

doc/api/fs.md

`fs`( -> `fs` (

in case of -> for

gibfahn · 2017-05-06T09:46:38Z

doc/api/fs.md

Maybe say < v6.3.0, otherwise < v6.3.x might be confusing.

gibfahn · 2017-05-06T09:50:58Z

FYI, at the moment your Git author name and email address are set to:

anshulguleria <anshulguleria@ymail.com>

People usually choose to use their full names for commits. To set your name globally you can do:

git config --global user.name "Anshul Guleria"

To change the author for a single commit you can do:

git commit --amend --author="Anshul Guleria <anshulguleria@ymail.com>"
git push --force-with-lease

If you don't want to have your name against this commit, that's fine too!

gibfahn · 2017-05-06T09:56:01Z

Also it might be worth using a different example constant, as it looks like R_OK is in fs and fs.constants (I tested with 6.10.1), so (fs.constants || fs).R_OK isn't technically needed.

> fs
{ constants: 
   { O_RDONLY: 0,
     O_WRONLY: 1,
     O_RDWR: 2,
     S_IFMT: 61440,
     S_IFREG: 32768,
     S_IFDIR: 16384,
     S_IFCHR: 8192,
     S_IFBLK: 24576,
     S_IFIFO: 4096,
     S_IFLNK: 40960,
     S_IFSOCK: 49152,
     O_CREAT: 512,
     O_EXCL: 2048,
     O_NOCTTY: 131072,
     O_TRUNC: 1024,
     O_APPEND: 8,
     O_DIRECTORY: 1048576,
     O_NOFOLLOW: 256,
     O_SYNC: 128,
     O_SYMLINK: 2097152,
     O_NONBLOCK: 4,
     S_IRWXU: 448,
     S_IRUSR: 256,
     S_IWUSR: 128,
     S_IXUSR: 64,
     S_IRWXG: 56,
     S_IRGRP: 32,
     S_IWGRP: 16,
     S_IXGRP: 8,
     S_IRWXO: 7,
     S_IROTH: 4,
     S_IWOTH: 2,
     S_IXOTH: 1,
     F_OK: 0,
     R_OK: 4,
     W_OK: 2,
     X_OK: 1 },
  F_OK: 0,
  R_OK: 4,
  W_OK: 2,
  X_OK: 1,
[...]

Sorry for all the nits!

anshulguleria · 2017-05-07T06:23:53Z

Hi, @gibfahn I choose (fs.constants || fs).R_OK because the documentation currently says R_OK is present in fs.constants but pre v6.3.0 it was on fs so if someone is using node engine which can be less or greater than the mentioned version, e.g. engine: '>5.10.0' then he can use that condition to be on safer side. Also its soft deprecation thus fs.R_OK might be removed later on, so directly doing fs.R_OK might not be future safe.

But if you still feel its not required I'll remove it. Thanks.

gibfahn · 2017-05-07T16:07:23Z

@anshulguleria okay, that seems reasonable, and it probably makes sense to leave R_OK as is. Thanks.

jasnell · 2017-05-09T18:17:45Z

doc/api/fs.md

Slight suggested modification:

The following constants are exported by `fs.constants` (or `fs` in case of Node.js `< v6.3.0`). *Note*: Not every constant will be available on every operating system.

(note the slight formatting difference on the Note part also.

Can you also change in case of -> for (in addition to @jasnell's suggested changes)?

Ok. Handled both cases.

jasnell

This LGTM but I do have a suggested improvement.

gibfahn · 2017-05-10T09:55:54Z

@cjihrig LGTY now?

cjihrig · 2017-05-10T13:31:03Z

The changes starting at line 2,695 seem unnecessary to me. The changelog referencing versions below 6.3.0 also seems unnecessary (it's kind of implied by being in a changelog).

anshulguleria · 2017-05-11T07:00:29Z

@cjihrig I think you are right. That does seems unnecessary, and it also kind of states that all constants stated below are accessible by fs for Node < 6.3.0 which is not true. Only the four main ones are accessible by fs. I will remove those changes. Thanks.

gibfahn · 2017-05-11T11:28:53Z

I reopened the issue that this fixes, so could you change the commit message:

-Refs: https://github.com/nodejs/node/issues/8044
+Fixes: https://github.com/nodejs/node/issues/8044

that way when this PR lands it will automatically close that issue.

Thanks for sticking with this! Should be good to land after this and @cjihrig's suggestion.

* Add changelog history in `fs.access` for the changes introduced to `constants` in `fs` module prior to Node `<v6.3.0`. Fixes: nodejs#8044

anshulguleria · 2017-05-11T14:41:45Z

Removed the unnecessary documentation as per @cjihrig suggestion and updated the commit for the issue fix as per @gibfahn .

gibfahn · 2017-05-11T15:18:57Z

@cjihrig LGTY?

gibfahn · 2017-06-11T19:52:43Z

Will land this in a bit if there are no objections.

gibfahn · 2017-06-11T20:49:15Z

Landed in b6e2cde, thanks for the PR @anshulguleria !

Add changelog history in `fs.access` for the changes introduced to `constants` in the `fs` module prior to Node v6.3.0. PR-URL: #12690 Fixes: #8044 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

MylesBorins · 2017-07-17T18:23:50Z

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

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 27, 2017

vsemozhetbyt reviewed Apr 27, 2017

View reviewed changes

anshulguleria force-pushed the fs_constants_doc_clarification branch 2 times, most recently from 1bd827e to 3b0c076 Compare April 27, 2017 10:22

cjihrig requested changes Apr 27, 2017

View reviewed changes

jasnell requested changes Apr 27, 2017

View reviewed changes

anshulguleria force-pushed the fs_constants_doc_clarification branch from 394af79 to dca0572 Compare May 3, 2017 15:18

anshulguleria force-pushed the fs_constants_doc_clarification branch from dca0572 to 4b95342 Compare May 5, 2017 09:15

gibfahn reviewed May 6, 2017

View reviewed changes

anshulguleria force-pushed the fs_constants_doc_clarification branch 2 times, most recently from 5dc1786 to b386905 Compare May 7, 2017 06:30

gibfahn approved these changes May 7, 2017

View reviewed changes

jasnell reviewed May 9, 2017

View reviewed changes

jasnell approved these changes May 9, 2017

View reviewed changes

anshulguleria force-pushed the fs_constants_doc_clarification branch from b386905 to 2871d6f Compare May 10, 2017 05:55

gibfahn mentioned this pull request May 11, 2017

Doc has mistake: The fs doesn't exist constants of property #8044

Closed

doc: fs constants for Node < v6.3.0 in fs.md

6312847

* Add changelog history in `fs.access` for the changes introduced to `constants` in `fs` module prior to Node `<v6.3.0`. Fixes: nodejs#8044

anshulguleria force-pushed the fs_constants_doc_clarification branch from 2871d6f to 6312847 Compare May 11, 2017 14:38

cjihrig approved these changes May 11, 2017

View reviewed changes

Merge branch 'master' into fs_constants_doc_clarification

be7beef

gibfahn self-assigned this Jun 11, 2017

gibfahn closed this Jun 11, 2017

anshulguleria deleted the fs_constants_doc_clarification branch June 12, 2017 11:58

addaleax mentioned this pull request Jun 12, 2017

v8.1.1 proposal #13598

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Uh oh!

Conversation

anshulguleria commented Apr 27, 2017

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt Apr 27, 2017 • 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!

anshulguleria commented Apr 27, 2017

Uh oh!

vsemozhetbyt commented Apr 27, 2017

Uh oh!

anshulguleria commented Apr 27, 2017

Uh oh!

cjihrig left a comment

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!

anshulguleria commented Apr 28, 2017

Uh oh!

cjihrig commented Apr 28, 2017

Uh oh!

anshulguleria commented May 2, 2017

Uh oh!

anshulguleria commented May 2, 2017

Uh oh!

cjihrig commented May 2, 2017

Uh oh!

anshulguleria commented May 3, 2017

Uh oh!

anshulguleria commented May 3, 2017

Uh oh!

addaleax commented May 3, 2017

Uh oh!

anshulguleria commented May 6, 2017

Uh oh!

jasnell commented May 6, 2017

Uh oh!

gibfahn left a comment

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!

gibfahn commented May 6, 2017

Uh oh!

gibfahn commented May 6, 2017

Uh oh!

anshulguleria commented May 7, 2017

Uh oh!

gibfahn commented May 7, 2017

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!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn commented May 10, 2017

Uh oh!

cjihrig commented May 10, 2017

vsemozhetbyt Apr 27, 2017 •

edited

Loading