Skip to content

tools: increase maximum line length to 120 characters - #41586

Merged
nodejs-github-bot merged 2 commits into
nodejs:masterfrom
Trott:max-len-120
Jan 21, 2022
Merged

tools: increase maximum line length to 120 characters#41586
nodejs-github-bot merged 2 commits into
nodejs:masterfrom
Trott:max-len-120

Conversation

@Trott

@TrottTrott commented Jan 19, 2022

Copy link
Copy Markdown
Member

No description provided.

@nodejs-github-botnodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jan 19, 2022
This was referenced Jan 19, 2022
Closed
Closed
@tniessen

tniessen commented Jan 19, 2022

Copy link
Copy Markdown
Member

Alternative: #41509
Alternative: #41536

@tniessen

tniessen commented Jan 19, 2022

Copy link
Copy Markdown
Member

Should this also apply to C++ sources and/or Markdown for consistency? (I'm not saying it's a good idea, but I am curious if the arguments for doing this in JavaScript are really specific to JavaScript.)

mcollina
mcollina approved these changes Jan 19, 2022
View reviewed changes

@mcollinamcollina left a comment

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.

lgtm

@Trott

Trott commented Jan 19, 2022

Copy link
Copy Markdown
MemberAuthor

Should this also apply to C++ sources and/or Markdown for consistency? (I'm not saying it's a good idea, but I am curious if the arguments for doing this in JavaScript are really specific to JavaScript.)

I'd prefer to treat each one separately so that decisions about one don't get complicated by considerations that only apply in one of the other contexts.

It might also be good to treat this as an experiment. If anyone is hesitant about increasing the limit, then maybe they'd be comfortable with just trying it for a few months and then deciding if we've had to give a lot of line-length nits or not.

But here's my take anyway on C++ and markdown:

  • The last time I programmed C++ on anything more than an occasional as-needed basis was around 20 years ago so I don't have an opinion there.

  • I can go either way with Markdown, but I think I favor eliminating the line breaks there too. All the same IDE (and markdown editor) arguments apply, and the argument that it will greatly simplify diffs applies much more strongly for prose than for code.

@jasnell

jasnell commented Jan 19, 2022

Copy link
Copy Markdown
Member

Increasing the c++ limit to 120 works for me.

bnb
bnb approved these changes Jan 19, 2022
View reviewed changes
@bnb

bnb commented Jan 19, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

I can go either way with Markdown, but I think I favor eliminating the line breaks there too. All the same IDE (and markdown editor) arguments apply, and the argument that it will greatly simplify diffs applies much more strongly for prose than for code.

Massively favor eliminating line breaks. In general, it seems that the ecosystem has adopted this as a standard, and it's extremely strange for us to be an outlier.

@tniessen

tniessen commented Jan 20, 2022

Copy link
Copy Markdown
Member

Massively favor eliminating line breaks. In general, it seems that the ecosystem has adopted this as a standard, and it's extremely strange for us to be an outlier.

On a side note, I suspect that most of the ecosystem doesn't need to backport documentation changes to multiple release lines with diverging commit histories, so they are probably less concerned about the increased number of conflicts and the reduced usefulness of git operations such as git blame.

@TrottTrott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2022
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 21, 2022
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@TrottTrott added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 21, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2022
@nodejs-github-bot

nodejs-github-bot commented Jan 21, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42064/

@nodejs-github-bot

nodejs-github-bot commented Jan 21, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42065/

@TrottTrott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 870044f into nodejs:masterJan 21, 2022
@nodejs-github-bot

nodejs-github-bot commented Jan 21, 2022

Copy link
Copy Markdown
Collaborator

Landed in 870044f

@Trott
Trott deleted the max-len-120 branch January 21, 2022 15:29
This was referenced Jan 22, 2022
Open
Open
Open
Open
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
@Trott@BethGriggs
tools: increase maximum line length to 120 characters
57ada37
PR-URL: #41586
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
@github-actionsgithub-actionsBot mentioned this pull request Jan 26, 2022
Open
26 tasks
@tniessentniessen mentioned this pull request Feb 7, 2022
Merged
@ruyadornoruyadorno mentioned this pull request Feb 8, 2022
Merged
danielleadams pushed a commit that referenced this pull request Feb 28, 2022
@Trott@danielleadams
tools: increase maximum line length to 120 characters
c346d78
PR-URL: #41586
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
@Trott@danielleadams
tools: increase maximum line length to 120 characters
ad1d870
PR-URL: #41586
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
@danielleadamsdanielleadams mentioned this pull request Mar 3, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@Trott@danielleadams
tools: increase maximum line length to 120 characters
a544321
PR-URL: #41586
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
@Trott@danielleadams
tools: increase maximum line length to 120 characters
cdbe291
PR-URL: #41586
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
@GeoffreyBoothGeoffreyBooth mentioned this pull request Oct 18, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@bnbbnbbnb approved these changes
@targostargostargos approved these changes
@BethGriggsBethGriggsBethGriggs approved these changes
+1 more reviewer
@MesteeryMesteeryMesteery approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Trott@tniessen@jasnell@bnb@nodejs-github-bot@mcollina@targos@BethGriggs@Mesteery