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: clarify 'change' in fs.watch and FSWatcher #7506

Conversation

claudiorodriguez
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

The 'change' event can refer to either the value of 'event'
in the fs.watch listener, and to an FSWatcher actual event.
This adds some clarification.

Fixes: #7504

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Jul 1, 2016
@@ -1447,6 +1453,9 @@ The listener callback gets two arguments `(event, filename)`. `event` is either
`'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
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the actual confusion in the original issue, is because of the previous paragraph. IINW, OP misunderstood the type of event (rename and change) as the actual events emitted. Perhaps we can change/explain the parameter event.

Thoughts @Trott?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thefourtheye I could change the name in both the code and docs to eventType or something like that - it's just a change of identifier name so it wouldn't be intrusive/breaking

Copy link
Member

Choose a reason for hiding this comment

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

I think changing event to eventType would be a significant improvement.

@Trott
Copy link
Member

Trott commented Jul 1, 2016

/cc @nodejs/documentation

The name 'event' for the argument of the listener in
fs.watch was confusing considering FSWatcher also had
events. This changes the name of the argument to
eventType.

Fixes: nodejs#7504
PR-URL: nodejs#7506
@claudiorodriguez
Copy link
Contributor Author

I've changed event to eventType in the docs and fs code, PTAL
CI: https://ci.nodejs.org/job/node-test-pull-request/3157/

@Trott
Copy link
Member

Trott commented Jul 2, 2016

event -> eventType change LGTM. Would want a documentation person's opinion on the rest. (It makes sense to me, but I'm not sure I'm thinking like a reader/end user.)

@thefourtheye
Copy link
Contributor

@claudiorodriguez
Copy link
Contributor Author

@nodejs/documentation any comments on the PR as it is now?

@claudiorodriguez
Copy link
Contributor Author

@Trott do you object to merging this for now? I think it's an improvement to the current situation, we can expect further copyedit PRs to improve on the doc part. I can also limit the change to just event -> eventType

@Trott
Copy link
Member

Trott commented Jul 13, 2016

@claudiorodriguez No objection from me! :shipit:

@claudiorodriguez
Copy link
Contributor Author

Landed in 6e15ae9

claudiorodriguez added a commit that referenced this pull request Jul 14, 2016
The name 'event' for the argument of the listener in
fs.watch was confusing considering FSWatcher also had
events. This changes the name of the argument to
eventType.

Fixes: #7504
PR-URL: #7506
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
evanlucas pushed a commit that referenced this pull request Jul 18, 2016
The name 'event' for the argument of the listener in
fs.watch was confusing considering FSWatcher also had
events. This changes the name of the argument to
eventType.

Fixes: #7504
PR-URL: #7506
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
The name 'event' for the argument of the listener in
fs.watch was confusing considering FSWatcher also had
events. This changes the name of the argument to
eventType.

Fixes: #7504
PR-URL: #7506
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorins
Copy link
Contributor

@claudiorodriguez thanks for adding the tag. Unfortunately this relies on some changes that have not yet landed. Please feel free to manually backport and open a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incomplete FSWatcher documentation.
6 participants