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.

Sign up for GitHub

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

Jump to bottom

doc: copyedit fs doc #3097

Closed
wants to merge 2 commits into from
Closed

doc: copyedit fs doc #3097

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 28, 2015

Minor editing for clarity and concision.

@Trott Trott added the doc Issues and PRs related to the documentations. label Sep 28, 2015
@thefourtheye
Copy link
Contributor

Recursive support for Windows has been added only recently and I assume this is going to be an ongoing process. So, we may not require that change.

cc @saghul

@Trott
Copy link
Member Author

Trott commented Sep 28, 2015

@thefourtheye I'm not sure I understand what you mean. The only change I'm proposing for that part of the doc is to change currently supported on OS X and Windows to only supported on OS X and Windows. I think that's an improvement because:

  • The word currently is superfluous. Whatever you put in the documentation, it is assumed to be the current state of the API.
  • The word only makes it explicit that it is not supported anywhere else. Leaving out only makes it ambiguous. Is it supported on OS X and Windows as well as other platforms? Or just those two platforms and nowhere else?

Am I misunderstanding something?

@thefourtheye
Copy link
Contributor

Hmmm okay. LGTM then

@targos
Copy link
Member

targos commented Sep 28, 2015

LGTM

@saghul
Copy link
Member

saghul commented Sep 28, 2015

IMHO it can be trimmed further.

"Only FSEvents supports this type of file watching so it is unlikely any additional platforms will be added soon."

Here "FSEvents" refers to the OSX API, but since we added Windows support I don't think we should say it's unlikely that we add more.

@Trott
doc: further copyediting to fs doc
a921dc9 
Removed "Only FSEvents supports this type of file watching so it is
unlikely any additional platforms will be added soon."

Per @saghul, "FSEvents" refers to the OSX API, but since we added
Windows support it may not be  unlikely that we add more.
@Trott
Copy link
Member Author

Trott commented Sep 28, 2015

@saghul OK, trimmed further. Thanks. How's it look now? /cc @thefourtheye @targos

@thefourtheye
Copy link
Contributor

LGTM

2 similar comments
@targos
Copy link
Member

targos commented Sep 28, 2015

LGTM

@saghul
Copy link
Member

saghul commented Sep 28, 2015

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Sep 30, 2015
@Trott
doc: copyedit fs.watch() information
0e4b772 
Removed "Only FSEvents supports this type of file watching so it is
unlikely any additional platforms will be added soon."

Per @saghul, "FSEvents" refers to the OSX API, but since we added
Windows support it may not be  unlikely that we add more.

PR-URL: nodejs#3097
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
@Trott
Copy link
Member Author

Trott commented Sep 30, 2015

Landed in 0e4b772

@Trott Trott closed this Sep 30, 2015
Trott added a commit that referenced this pull request Oct 2, 2015
@Trott @rvagg
doc: copyedit fs.watch() information
928166c 
Removed "Only FSEvents supports this type of file watching so it is
unlikely any additional platforms will be added soon."

Per @saghul, "FSEvents" refers to the OSX API, but since we added
Windows support it may not be  unlikely that we add more.

PR-URL: #3097
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
@rvagg rvagg mentioned this pull request Oct 3, 2015
Closed
@Trott Trott deleted the doc-fs branch January 13, 2022 22:28
@aliscco aliscco mentioned this pull request Jun 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Trott @thefourtheye @targos @saghul