FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: add Buffer.lastIndexOf by dcposch · Pull Request #4933 · nodejs/node · GitHub

/ node Public

doc: add Buffer.lastIndexOf - #4933

Closed
dcposch wants to merge 1 commit into
nodejs:masterfrom
dcposch:doc
ConversationCommits (1)ChecksFiles changed
Closed

doc: add Buffer.lastIndexOf#4933
dcposch wants to merge 1 commit into
nodejs:masterfrom
dcposch:doc

Conversation

dcposch commented Jan 28, 2016

Copy link
Copy Markdown
Contributor

Complements #4604

Together they fix #4846

tflanagan commented Jan 28, 2016

Copy link
Copy Markdown
Contributor

Can you order this alphabetically like the rest?

r-52 added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Jan 28, 2016
dcposch force-pushed the doc branch 3 times, most recently from b80c2dc to a31e635 Compare January 28, 2016 21:23

dcposch commented Jan 28, 2016

Copy link
Copy Markdown
Contributor Author

@tflanagan fixed

tflanagan commented Jan 28, 2016

Copy link
Copy Markdown
Contributor

@dcposch, thanks :)

Qard commented Jan 28, 2016

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

@Qard fixed

TimothyGu reviewed Jan 29, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown

TimothyGu Jan 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

UTF**-**8

TimothyGu Jan 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also you should need to mention the default encoding at all since it is listed above.

dcposch Jan 29, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's rendered "UTF8" everywhere else in this file. I think this is fine.

dcposch force-pushed the doc branch 2 times, most recently from 5d3105a to 82733a2 Compare February 1, 2016 02:52

dcposch commented Feb 1, 2016

Copy link
Copy Markdown
Contributor Author

@Qard looks more dece now?

Qard reviewed Feb 1, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown Outdated

Qard Feb 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you wrap each type separately, please?

dcposch Feb 1, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

dcposch Feb 1, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Qard added a commit that wraps types separately everywhere there's a list

TimothyGu Feb 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unfortunately wrapping the three separately confuses the JSON documentation generation IIRC.

jasnell commented Feb 1, 2016

Copy link
Copy Markdown
Member

LGTM

dcposch commented Feb 2, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell thanks for taking a look! Someone else will have to merge it for me, ideally at the same time as #4846 once that is ready.

dcposch changed the title Add docs for Buffer.lastIndexOf doc: add Buffer.lastIndexOf Feb 4, 2016
tflanagan reviewed Feb 12, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown Outdated

tflanagan Feb 12, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you please add

const Buffer = require('buffer').Buffer;

targos Feb 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it needed for the linter ? Buffer is global.

tflanagan Feb 12, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The linter does complain about it

dcposch Feb 12, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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

Copy link
Copy Markdown
Contributor Author

@jasnell thanks for the review.

i just rebased because there were conflicts. the PR is even smaller now -- it only adds the Buffer.lastIndexOf section to the docs and doesn't modify any existing sections

dcposch commented Mar 2, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell @Qard let me know if the docs look good now.

I think the code part ( #4846 ) is ready to go.

jasnell commented Mar 2, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell commented Mar 2, 2016

Copy link
Copy Markdown
Member

how does this relate to #4846?

dcposch commented Mar 3, 2016

Copy link
Copy Markdown
Contributor Author

@jasnell #4646 adds a new feature. This PR adds the corresponding docs.

jasnell commented Mar 3, 2016

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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.

dcposch mentioned this pull request Mar 4, 2016
Closed

dcposch commented Mar 4, 2016

Copy link
Copy Markdown
Contributor Author

@Qard done. Moved to #4846

dcposch closed this Mar 4, 2016
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL