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.

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

tools: update to ESLint 4.12.0 #16948

Merged
merged 1 commit into from
Nov 28, 2017
Merged

tools: update to ESLint 4.12.0 #16948

merged 1 commit into from
Nov 28, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 11, 2017

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

tools

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

@jasnell jasnell 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.

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

cjihrig commented Nov 26, 2017

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

Trott commented Nov 26, 2017

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

@Trott Trott 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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 28, 2017

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

None of the failures look related. Landing this.

@cjihrig cjihrig merged commit c2d738d into nodejs:master Nov 28, 2017
@cjihrig cjihrig 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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16948
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16948
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins 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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@gibfahn gibfahn 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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants