-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
fs: fix link rot in fs.md #8940
Conversation
Updated line 2197
@@ -2194,7 +2194,7 @@ The following constants are meant for use with the [`fs.Stats`][] object's | |||
[MDN-Date]: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Date | |||
[Readable Stream]: stream.html#stream_class_stream_readable | |||
[Writable Stream]: stream.html#stream_class_stream_writable | |||
[inode]: http://www.linux.org/threads/intro-to-inodes.4130 | |||
[inode]:https://en.wikipedia.org/wiki/Inode |
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.
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 you need a space after the :
?
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.
Yes I merged a pull request for the same
Also could you change the commit message to be more like: |
fs: fix link rot in fs.md
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.
Looks good! Squashing the commits + fixing up the commit message can be done when merging the PR.
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.
LGTM
By the way, @ss22ever could you double check on others which we can use wikipedia instead of? |
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.
LGTM
PR-URL: #8940 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Landed in 3945943. Thank you! |
PR-URL: #8940 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Yorki as told by you I double checked the links the rest of them are working fine here. |
@ss22ever I'm not sure but I think @yorkie wanted to check if there were better resources than Wikipedia. |
@lpinca oh pardon then if thats so should I make a PR for the ones that I have found? |
fwiw, I read @yorkie’s comment the opposite way… i.e. checking which other links could be replaced by links to Wikipedia. 😄 |
Yeah even I and then I checked all the links to ensure that :D @addaleax |
2 vs 1 I'm probably the one that misread / did not understand 😄. @ss22ever I think that it is fine as is, maybe @yorkie can clarify later. |
Is this PR merged ? If yes why can I not see the merged icon ? |
@ss22ever Yes! “Landed in 3945943” means that it’s been merged and that the commit in which it has been merged into commit 3945943 on master. The “Merged” icon is missing for technical reasons related to our process of merging PRs. |
PR-URL: #8940 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Checklist
Affected core subsystem(s)
doc
Description of change
Updated line 2197
fixes #8825