-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Conversation
@@ -571,7 +571,7 @@ no-op, not an error. | |||
|
|||
## fs.watch(filename[, options][, listener]) | |||
|
|||
Stability: 2 - Unstable. | |||
Stability: 2 - Stable. |
There was a problem hiding this comment.
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.
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. |
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.) |
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). |
I'd move to remove per-function stability index across the docs. Is this the only spot? |
@Fishrock123 yes, besides deprecation notices, fs is the only culprit. |
|
Back to |
@Trott that sounds like a good plan. |
@brendanashworth What about patching |
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. |
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]) |
@ChALkeR the behavior would probably change for |
@ChALkeR We aren't actually deprecating it. |
@Fishrock123 I never said anything about deprecating here. @brendanashworth How would that break backwards compatibility? Do the modules rely on how is |
… in main doc text, expand advisory with brief explanation
Agree with @Fishrock123 on styling to set the note off from the main text a bit. Added a new commit for it. |
Fixes: #1754 PR-URL: #1775 Reviewed-By: Brendan Ashworth <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Thanks, landed in eb1856d |
Fixes: nodejs/node#1754 PR-URL: nodejs/node#1775 Reviewed-By: Brendan Ashworth <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Addresses #1754