docs: clarify eventType in fs.watch #9318

seishun · 2016-10-27T17:52:09Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

'rename' is confusing, and it's not clear what "they" refers to.

Fixes: #9082

'rename' is confusing, and it's not clear what "they" refers to. Fixes: #9082

Trott

A few style nits that don't really matter much, but more importantly: I don't think the information about rename is correct on all operating systems. I could be misreading what's going on, but judging from some of the tests, it appears that whether you get change or rename on file creation (for example) is OS-dependent. See, for example:

node/test/sequential/test-fs-watch.js

Line 83 in 6893f3c

var renameEv = common.isSunOS ? 'change' : 'rename';

Trott · 2016-10-28T03:23:54Z

doc/api/fs.md

@@ -1702,8 +1702,11 @@ The listener callback gets two arguments `(eventType, filename)`.  `eventType` i
 `'rename'` or `'change'`, and `filename` is the name of the file which triggered
 the event.

-Please note the listener callback is attached to the `'change'` event
-fired by [`fs.FSWatcher`][], but they are not the same thing.
+Note that `'rename'` is also emitted when a file is deleted or added - in other words,


Nit: no hyphen. ...when a file is deleted or added. In other words, ...

Trott · 2016-10-28T03:26:54Z

doc/api/fs.md

-Please note the listener callback is attached to the `'change'` event
-fired by [`fs.FSWatcher`][], but they are not the same thing.
+Note that `'rename'` is also emitted when a file is deleted or added - in other words,
+it's emitted whenever a filename appears or disappears in the directory.


Nit: it's -> it is

Trott · 2016-10-28T03:27:06Z

doc/api/fs.md

+it's emitted whenever a filename appears or disappears in the directory.
+
+Also note the listener callback is attached to the `'change'` event fired by
+[`fs.FSWatcher`][], but it's not the same thing as the `'change'` value of `eventType`.


Nit: it's -> it is

seishun · 2016-10-28T12:29:16Z

Fixed the nits.

According to my tests, this behavior is consistent across Windows, Linux and MacOS, so I just changed it to "most platforms".

Trott

A couple more nits, but LGTM

Trott · 2016-10-28T14:46:29Z

doc/api/fs.md

@@ -1702,8 +1702,11 @@ The listener callback gets two arguments `(eventType, filename)`.  `eventType` i
 `'rename'` or `'change'`, and `filename` is the name of the file which triggered
 the event.

-Please note the listener callback is attached to the `'change'` event
-fired by [`fs.FSWatcher`][], but they are not the same thing.
+Note that on most platforms, `'rename'` is also emitted when a file is deleted or added.


Nit: remove also

I used "also" because it's in addition to being renamed.

I suggested removing it because I think it could be interpreted to mean that both a rename and a change event will be emitted.

I think someone might think it means "A change event is emitted and a rename event is also emitted."

I understand that you mean it as "A rename event is emitted in this case and a rename event is also emitted in this other case."

I'm fine with leaving it the way you have it if you have a strong preference for that. But if it doesn't make much of a difference to you either way, I'd prefer it removed.

I feel like with your suggestion the two sentences somewhat contradict each other...

How about a compromise? Just combine them into one sentence: "Note that on most platforms, 'rename' is emitted whenever a filename appears or disappears in the directory."

Yes, that works for me.

Trott · 2016-10-28T14:47:09Z

doc/api/fs.md

-Please note the listener callback is attached to the `'change'` event
-fired by [`fs.FSWatcher`][], but they are not the same thing.
+Note that on most platforms, `'rename'` is also emitted when a file is deleted or added.
+In other words, it is emitted whenever a filename appears or disappears in the directory.


Nit: add on most platforms here too to be as explicit as possible: In other words, on most platforms, it is emitted...

jasnell

LGTM with a nit

jasnell · 2016-10-30T16:21:52Z

doc/api/fs.md

+or disappears in the directory.
+
+Also note the listener callback is attached to the `'change'` event fired by
+[`fs.FSWatcher`][], but it is not the same thing as the `'change'` value of `eventType`.


nit: long line here.

silverwind · 2016-11-15T22:12:32Z

Thanks! Landed in b209a6e.

'rename' is confusing, and it's not clear what "they" refers to. Fixes: #9082 PR-URL: #9318 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins · 2016-12-13T20:53:15Z

@seishun is this behavior consistent in v4 and v6? Should this be backported?

seishun · 2016-12-14T17:53:54Z

@thealphanerd AFAIK this behavior has been the same since fs.watch was introduced.

'rename' is confusing, and it's not clear what "they" refers to. Fixes: #9082 PR-URL: #9318 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

docs: clarify eventType in fs.watch

d627fc2

'rename' is confusing, and it's not clear what "they" refers to. Fixes: #9082

nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Oct 27, 2016

Trott requested changes Oct 28, 2016

View reviewed changes

nits

7591c57

Trott approved these changes Oct 28, 2016

View reviewed changes

more nits

54cd5ec

jasnell approved these changes Oct 30, 2016

View reviewed changes

hopefully last nit

6a88f95

silverwind approved these changes Nov 15, 2016

View reviewed changes

silverwind closed this Nov 15, 2016

seishun deleted the rename branch December 6, 2016 10:08

MylesBorins added lts-watch-v4.x labels Dec 13, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Dec 21, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V6.9.3 proposal #10394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: clarify eventType in fs.watch #9318

docs: clarify eventType in fs.watch #9318

seishun commented Oct 27, 2016

Trott left a comment

Trott Oct 28, 2016

Trott Oct 28, 2016

Trott Oct 28, 2016

seishun commented Oct 28, 2016

Trott left a comment

Trott Oct 28, 2016

seishun Oct 28, 2016

Trott Oct 29, 2016

seishun Oct 29, 2016 •

edited

Loading

Trott Oct 29, 2016

Trott Oct 28, 2016

jasnell left a comment

jasnell Oct 30, 2016

silverwind commented Nov 15, 2016

MylesBorins commented Dec 13, 2016

seishun commented Dec 14, 2016

docs: clarify eventType in fs.watch #9318

docs: clarify eventType in fs.watch #9318

Conversation

seishun commented Oct 27, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun commented Oct 28, 2016

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun Oct 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Nov 15, 2016

MylesBorins commented Dec 13, 2016

seishun commented Dec 14, 2016

seishun Oct 29, 2016 •

edited

Loading