docs: clarify eventType in fs.watch by seishun · Pull Request #9318 · nodejs/node

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


-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


-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

Uh oh!

Conversation

seishun commented Oct 27, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott 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!

seishun commented Oct 28, 2016

Uh oh!

Trott 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!

seishun Oct 29, 2016 • 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!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Nov 15, 2016

Uh oh!

MylesBorins commented Dec 13, 2016

Uh oh!

seishun commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

seishun Oct 29, 2016 •

edited

Loading