Skip to content

Conversation

@cjihrig
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

@nodejs-github-botnodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 11, 2017
Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

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

Rubber stamp lgtm

@Trott
Copy link
Member

Trott commented Nov 13, 2017

I don't object, but in the past, we've refrained from updating unless it fixed a bug that affected us or provided a feature that we wanted to use. That said, I'm not arguing "We've always done it this way, so we must always do it this way." If we'd rather keep our dependencies more up-to-date more frequently, hey, cool.

@cjihrigcjihrig changed the title tools: update to ESLint 4.11.0tools: update to ESLint 4.12.0Nov 26, 2017
@cjihrig
Copy link
ContributorAuthor

I've updated this to 4.12.0. This version comes with a new rule, implicit-arrow-linebreak. I'm not sure if this is a rule we'd want to enable or not. In theory, it looks good, but we have 51 violations that are probably all due to long lines.

@Trott
Copy link
Member

I'm not sure if this is a rule we'd want to enable or not.

My personal rule of thumb has evolved to be: If people leave nits about it or if people ask explicitly about it, then it needs a rule. Otherwise, no need to enable a rule that no one is otherwise concerned with.

So I'd say no need to enable it at this time. (But I don't feel strongly about it if someone else disagrees.)

Copy link
Member

@TrottTrott left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

PR-URL: nodejs#16948 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@cjihrig
Copy link
ContributorAuthor

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

None of the failures look related. Landing this.

@cjihrigcjihrig merged commit c2d738d into nodejs:masterNov 28, 2017
@cjihrigcjihrig deleted the eslint branch November 28, 2017 03:01
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16948 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16948 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16948 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
PR-URL: #16948 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #16948 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@cjihrig@Trott@jasnell@MylesBorins@gibfahn@nodejs-github-bot