| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Could you change this method to the _read() implementation?
Sorry, something went wrong.
|
Great work! Once the wording nit is corrected, this LGTM. We usually leave PRs open once they've been LGTM'd for about 48 hours to make sure all of the contributors get a chance to review. If no one has objections it should be merged shortly after that. Thanks for the contribution! |
Sorry, something went wrong.
There was a problem hiding this comment.
Can't we simply say 'it returns'?
Sorry, something went wrong.
|
@thefourtheye BTW, I was wondering. In this file there are a lot of duplicates and in others which I worked on too. Maybe replace those where possible by some default except places where arguments matter to the context of sentences (like "for this case use read(0)"). If so, what would be default stream.write(), write() or smth. different? I find stream.write() more appropriate because it equals to name of header where this method is described |
Sorry, something went wrong.
|
@estliberitas Yes, I was about to talk to you about the same (I couldn't earlier because I was on Mobile). What we probably can do is, use stream.write and stream.read consistently. And whenever we see the call with specific arguments, we can say something like "for this case use [stream.read()][] with 0". May be we can add a small description about the parameter being passed. What do you think? |
Sorry, something went wrong.
|
@thefourtheye Well, I find it a good idea. And as for multiple arguments, we just separate by comma? Like, for example, lines 1582-1583:
To me it's not so cool, but at least it's informative and saves us from copy-pasting a lot. Other concern is optional arguments:
|
Sorry, something went wrong.
|
@thefourtheye Other idea would be patch doc tool to look for links without arguments 😉 |
Sorry, something went wrong.
|
Or we can use a common name like this [stream.read(0)][stream-read] and then we can tag all the links with various arguments with [stream-read] |
Sorry, something went wrong.
|
@thefourtheye Wow, I got something new today. Thanks, I think that resolves all the questions above. |
Sorry, something went wrong.
|
@chrisdickinson @thefourtheye Guys, please check once have a time. To simplify life I made a small script to check links consistency: https://gist.github.com/estliberitas/bfd35c7a5a977aa5d221 |
Sorry, something went wrong.
|
Sweeeet :-) |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
Please go ahead and submit PRs. I always wanted to clean this thing myself. Glad you are doing this :-) |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
|
@estliberitas ... there have been some other changes that touch on the same file, can I ask you to rebase this and update so we can remove the duplication? Thank you! |
Sorry, something went wrong.
|
Yep, I guess it's #5007, ok will do now. |
Sorry, something went wrong.
Add missing links, remove duplicate ones, fix constants and functions styling. Minor lexical corrections.
|
LGTM. Thank you! |
Sorry, something went wrong.
Add missing links, remove duplicate ones, fix constants and functions styling. Minor lexical corrections. PR-URL: #5009 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add missing links, remove duplicate ones, fix constants and functions styling. Minor lexical corrections. PR-URL: #5009 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add missing links, remove duplicate ones, fix constants and functions styling. Minor lexical corrections. PR-URL: #5009 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add missing links, remove duplicate ones, fix constants and functions styling. Minor lexical corrections. PR-URL: #5009 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add missing links, remove duplicate ones, fix constants and functions styling. Minor lexical corrections. PR-URL: #5009 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Add missing links, remove duplicate ones, fix constants and functions styling. Minor lexical corrections. PR-URL: nodejs#5009 Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
| Back | FazBrowse Home | New Git URL |
Add links, fix constants and functions styling. Minor lexical corrections.