Skip to content

Conversation

@Trott
Copy link
Member

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-botnodejs-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
MemberAuthor

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

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

@mcollinamcollina 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
Copy link
Member

@mcollinamcollina 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
MemberAuthor

@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
MemberAuthor

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.

@TrottTrott 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
MemberAuthor

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.

@TrottTrott added the wip Issues and PRs that are still a work in progress. label Mar 24, 2018
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.

@TrottTrottforce-pushed the max-line branch 2 times, most recently from 66c9e5d to fa7b2c3CompareMarch 26, 2018 23:24
@TrottTrottforce-pushed the max-line branch 7 times, most recently from d425e0d to ec7b5a7CompareApril 7, 2018 04:13
@BridgeARBridgeAR dismissed mcollina’s stale reviewApril 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

Ping @Trott

@Trott
Copy link
MemberAuthor

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.

@TrottTrottforce-pushed the max-line branch 4 times, most recently from 94cb561 to 18e5b0cCompareApril 14, 2018 21:30
@TrottTrottforce-pushed the max-line branch 3 times, most recently from a54da8e to c6ef2a2CompareApril 20, 2018 04:04
@TrottTrott removed the wip Issues and PRs that are still a work in progress. label Apr 20, 2018
@Trott
Copy link
MemberAuthor

@TrottTrott changed the title doc: wrap buffer.md at 80 characatersdoc: wrap buffer.md at 80 charactersApr 20, 2018
Wrap `buffer.md` at 80 characters and enforce with linter.
@Trott
Copy link
MemberAuthor

Trott added a commit to Trott/io.js that referenced this pull request Apr 20, 2018
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
MemberAuthor

Landed in 743341d

@TrottTrott closed this Apr 20, 2018
jasnell pushed a commit that referenced this pull request Apr 20, 2018
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]>
@TrottTrott deleted the max-line branch January 13, 2022 22:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.docIssues and PRs related to the documentations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

13 participants

@Trott@apapirovski@mcollina@BridgeAR@danbev@jasnell@lpinca@cjihrig@tniessen@hiroppy@vsemozhetbyt@gibfahn@nodejs-github-bot