Uh oh!
There was an error while loading. Please reload this page.
doc: add Buffer.lastIndexOf - #4933
Conversation
tflanagan
commented
Jan 28, 2016
Can you order this alphabetically like the rest? |
b80c2dc to
a31e635Comparedcposch
commented
Jan 28, 2016
@tflanagan fixed |
tflanagan
commented
Jan 28, 2016
@dcposch, thanks :) |
Qard
commented
Jan 28, 2016
Also, it'd be good to expand the parameter bits to be in-line with this other buffers PR: #4873 |
dcposch
commented
Jan 28, 2016
@Qard fixed |
There was a problem hiding this comment.
Also you should need to mention the default encoding at all since it is listed above.
There was a problem hiding this comment.
It's rendered "UTF8" everywhere else in this file. I think this is fine.
5d3105a to
82733a2Comparedcposch
commented
Feb 1, 2016
@Qard looks more dece now? |
There was a problem hiding this comment.
Can you wrap each type separately, please?
There was a problem hiding this comment.
It's done like this everywhere else in the file -- eg indexOf, includes, and fill
I can change all of those places if you want--LMK
There was a problem hiding this comment.
@Qard added a commit that wraps types separately everywhere there's a list
There was a problem hiding this comment.
Unfortunately wrapping the three separately confuses the JSON documentation generation IIRC.
jasnell
commented
Feb 1, 2016
LGTM |
dcposch
commented
Feb 2, 2016
There was a problem hiding this comment.
Could you please add
constBuffer=require('buffer').Buffer;There was a problem hiding this comment.
Is it needed for the linter ? Buffer is global.
There was a problem hiding this comment.
The linter does complain about it
There was a problem hiding this comment.
@tflanagan no other code sample in this doc does that
which linter are you talking about? make test and make doc both run without errors
dcposch
commented
Feb 19, 2016
@jasnell thanks for the review. i just rebased because there were conflicts. the PR is even smaller now -- it only adds the |
dcposch
commented
Mar 2, 2016
jasnell
commented
Mar 2, 2016
LGTM |
jasnell
commented
Mar 2, 2016
how does this relate to #4846? |
dcposch
commented
Mar 3, 2016
jasnell
commented
Mar 3, 2016
That's what I figured... any reason not to simply bundle the two commits into the single PR #4846? Would make it easier to land. |
Qard
commented
Mar 3, 2016
Agreed, it's better that docs commits of new features are included in the PR of the feature itself and just mention @nodejs/documentation in the issue to summon us docs folks for review. |
Complements #4604
Together they fix#4846