Skip to content
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.

Sign up for GitHub

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

Jump to bottom

doc: wrap buffer.md at 80 characters #19546

Closed
wants to merge 1 commit into from
Closed

doc: wrap buffer.md at 80 characters #19546

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 23, 2018

Wrap buffer.md at 80 characters and enforce with linter.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Mar 23, 2018
@Trott
Copy link
Member Author

Trott commented Mar 23, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/298/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 23, 2018
@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 23, 2018
@apapirovski
Copy link
Member

ping @mcollina given the objections last time this came up.

@mcollina mcollina removed the fast-track PRs that do not need to wait for 48 hours to land. label Mar 23, 2018
@mcollina
Copy link
Member

I'm still -1 for the time being on this. Happy for this go to a TSC vote if you feel strongly about it, this has been discussed extensively already and not linting this file was the middle ground that was reached. See the long discussion in #18726.

Thanks @apapirovski for the ping.

mcollina
mcollina previously requested changes Mar 23, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

making my -1 prominent

@Trott
Copy link
Member Author

Trott commented Mar 23, 2018

@mcollina Your opposition is based on the fact that the whitespace changes will complicate backporting? Or are there other concerns too?

@mcollina
Copy link
Member

I’m generically -1 on massive linting changes if I get the chance to review them. Regarding this specifically, yes it is a backport problem.

@Trott
Copy link
Member Author

Trott commented Mar 23, 2018

Regarding this specifically, yes it is a backport problem.

If I open backport PRs right now for 9.x, 8.x, and 6.x, would that be enough to remove your objection? Or is the churn here still just too much?

@mcollina
Copy link
Member

IMHO it is too much churn.
However I see that I should have pushed back way more strongly when the rule was introduced (and brought it to the TSC then). At this point this can land. There is a overwhelming response that this is wanted.

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 24, 2018
@BridgeAR
Copy link
Member

@Trott if I understood @mcollina correct he is actually OK with landing this because there are a lot of +1 on this. Can you verify that again @mcollina?

@mcollina
Copy link
Member

As I said, I should have brought the matter to the TSC back then. Anyway, feel free to land.

@Trott
Copy link
Member Author

Trott commented Mar 24, 2018

There's changes in here that break links. Still need to address those. So please don't land. I'll add the in progress label.

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Mar 24, 2018
gibfahn
gibfahn reviewed Mar 26, 2018
View reviewed changes
doc/api/buffer.md Outdated
* [`Buffer.from(array)`] returns a new `Buffer` containing a *copy* of the
provided octets.
* [`Buffer.from(arrayBuffer[, byteOffset [, length]])`]
[`Buffer.from(arrayBuffer)`] returns a new `Buffer` that *shares* the same
Copy link
Member

Choose a reason for hiding this comment

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

@Trott I could be wrong, but I think the broken links can be fixed by moving the opening [ up to the previous line.

@Trott Trott force-pushed the max-line branch 2 times, most recently from 66c9e5d to fa7b2c3 Compare March 26, 2018 23:24
@Trott
Copy link
Member Author

Trott commented Mar 30, 2018

@Trott is this is WIP?

@BridgeAR Yes. I have some smaller PRs against this file open that I want to land first.

(If having it remain open for a bit longer is problematic, it can be closed. I can always re-open it when it's ready.)

@Trott Trott force-pushed the max-line branch 7 times, most recently from d425e0d to ec7b5a7 Compare April 7, 2018 04:13
@BridgeAR BridgeAR dismissed mcollina’s stale review April 9, 2018 17:32

Dismissing to indicate that it is fine to land this PR as mentioned in a former comment.

@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

Ping @Trott

@Trott
Copy link
Member Author

Trott commented Apr 9, 2018

@BridgeAR Working through this slowly and opening other PRs when I find bigger issues that need addressing near one of these line-wraps. As I rebase, the change set gets smaller and smaller, which I think is A Good Thing. If having this open is a problem, feel free to close it. Otherwise, it's mildly convenient for me and it will land eventually, just not this week.

@Trott Trott force-pushed the max-line branch 4 times, most recently from 94cb561 to 18e5b0c Compare April 14, 2018 21:30
@Trott Trott force-pushed the max-line branch 3 times, most recently from a54da8e to c6ef2a2 Compare April 20, 2018 04:04
@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Apr 20, 2018
@Trott
Copy link
Member Author

Trott commented Apr 20, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/554/

@Trott Trott changed the title doc: wrap buffer.md at 80 characaters doc: wrap buffer.md at 80 characters Apr 20, 2018
@Trott
doc: wrap buffer.md at 80 characters
7373b7d 
Wrap `buffer.md` at 80 characters and enforce with linter.
@Trott
Copy link
Member Author

Trott commented Apr 20, 2018

Lite CI: https://ci.nodejs.org/job/node-test-commit-lite/724/

Trott added a commit to Trott/io.js that referenced this pull request Apr 20, 2018
@Trott
doc: wrap buffer.md at 80 characters
743341d 
Wrap `buffer.md` at 80 characters and enforce with linter.

PR-URL: nodejs#19546
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@Trott
Copy link
Member Author

Trott commented Apr 20, 2018

Landed in 743341d

@Trott Trott closed this Apr 20, 2018
jasnell pushed a commit that referenced this pull request Apr 20, 2018
@Trott @jasnell
doc: wrap buffer.md at 80 characters
9495d94 
Wrap `buffer.md` at 80 characters and enforce with linter.

PR-URL: #19546
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@Trott Trott deleted the max-line branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@hiroppy hiroppy hiroppy approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@mcollina mcollina mcollina left review comments

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.