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: stability index 2 = stable in fs.markdown, fixes #1754 #1775

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 23, 2015

Addresses #1754

@Trott Trott changed the title stability index 2 = stable stability index 2 = stable, fixes #1754 May 23, 2015
@mscdex mscdex added the doc Issues and PRs related to the documentations. label May 23, 2015
@@ -571,7 +571,7 @@ no-op, not an error.

## fs.watch(filename[, options][, listener])

Stability: 2 - Unstable.
Stability: 2 - Stable.
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 this line can be removed altogether - APIs are by default stable.

@silverwind
Copy link
Contributor

Also make sure the commit title (and probably the PR title) and message follows the guideline here: https://github.com/nodejs/io.js/blob/v1.x/CONTRIBUTING.md#step-3-commit.

@Trott Trott changed the title stability index 2 = stable, fixes #1754 doc: stability index 2 = stable in fs.markdown, fixes #1754 May 23, 2015
@Trott
Copy link
Member Author

Trott commented May 23, 2015

Amended my previous commit and changed the name of this commit per @silverwind.

Added a commit to remove redundant stability index indication per @brendanashworth. (Are the other two redundant as well? There's a stability index indication at the top that would seemingly apply to everything.)

@brendanashworth
Copy link
Contributor

perhaps @bnoordhuis could share some wisdom on that? my intuition says we move the recommendation into the text, out of the stability note (and remove that).

@Fishrock123
Copy link
Contributor

I'd move to remove per-function stability index across the docs. Is this the only spot?

@brendanashworth
Copy link
Contributor

@Fishrock123 yes, besides deprecation notices, fs is the only culprit.

@Trott
Copy link
Member Author

Trott commented May 24, 2015

domain.markdown is a curious case. The entire API is stability index 0 (deprecated), but there's an additional inline stability index 0 notice on domain.dispose().

@Trott
Copy link
Member Author

Trott commented May 24, 2015

Back to fs.watch vs fs.watchFile... I wonder if it would be useful to provide a single sentence explaining why fs.watch is preferred. Maybe summarize http://tech.nitoyon.com/en/blog/2013/10/02/node-watch-impl/ in a sentence or two.

@brendanashworth
Copy link
Contributor

@Trott that sounds like a good plan.

@ChALkeR
Copy link
Member

ChALkeR commented May 24, 2015

@brendanashworth What about patching fs.watchFile instead so it first tries to use the native fs notify api (as fs.watch does now) and if it fails, fallback to the current polling (as fs.watchFile does now)? I can open a separate issue for that.
Edit: btw, see https://github.com/paulmillr/chokidar

@Trott
Copy link
Member Author

Trott commented May 24, 2015

I pushed a new commit to make the changes described. I think (except for stuff that should go in separate tickets) I'm all caught up with the comments.

@Fishrock123
Copy link
Contributor

It'd be slightly better if this was a little more in line with how 86dd244 was. You can apply the patch below if you'd like, otherwise I'll just land this with it.

diff --git a/doc/api/fs.markdown b/doc/api/fs.markdown
index 176a0e9..92f34db 100644
--- a/doc/api/fs.markdown
+++ b/doc/api/fs.markdown
@@ -556,9 +556,9 @@ These stat objects are instances of `fs.Stat`.
 If you want to be notified when the file was modified, not just accessed
 you need to compare `curr.mtime` and `prev.mtime`.

-`fs.watch` is more efficient than `fs.watchFile` and `fs.unwatchFile`.
+_Note: `fs.watch` is more efficient than `fs.watchFile` and `fs.unwatchFile`.
 `fs.watch` should be used instead of `fs.watchFile` and `fs.unwatchFile`
-when possible.
+when possible._

 ## fs.unwatchFile(filename[, listener])

@@ -569,9 +569,9 @@ have effectively stopped watching `filename`.
 Calling `fs.unwatchFile()` with a filename that is not being watched is a
 no-op, not an error.

-`fs.watch` is more efficient than `fs.watchFile` and `fs.unwatchFile`.
+_Note: `fs.watch` is more efficient than `fs.watchFile` and `fs.unwatchFile`.
 `fs.watch` should be used instead of `fs.watchFile` and `fs.unwatchFile`
-when possible.
+when possible._

 ## fs.watch(filename[, options][, listener])

@brendanashworth
Copy link
Contributor

@ChALkeR the behavior would probably change for fs.watchFile, and it'd break backwards compatibility. It is probably best to note it in the docs and let it sit for a while.

@Fishrock123
Copy link
Contributor

@ChALkeR We aren't actually deprecating it.

@ChALkeR
Copy link
Member

ChALkeR commented May 25, 2015

@Fishrock123 I never said anything about deprecating here.

@brendanashworth How would that break backwards compatibility? Do the modules rely on how is fs.watchFile implemented internally? I propose to keep the API but to switch it internally to more efficient way of watching a file when it is possible, and when it's not — fallback to the current polling implementation. Even interval could be faked (by delaying the notification) to keep the API completely compatible.

@Trott
Copy link
Member Author

Trott commented May 26, 2015

Agree with @Fishrock123 on styling to set the note off from the main text a bit. Added a new commit for it.

Fishrock123 pushed a commit that referenced this pull request May 26, 2015
Fixes: #1754
PR-URL: #1775
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123
Copy link
Contributor

Thanks, landed in eb1856d

This was referenced May 27, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Fixes: nodejs/node#1754
PR-URL: nodejs/node#1775
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Trott Trott deleted the stableunstable branch October 14, 2021 13:47
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants