Skip to content

tools: update ESLint to v4.0.0 - #13645

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:eslint-v4.0.0
Closed

tools: update ESLint to v4.0.0#13645
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:eslint-v4.0.0

Conversation

@Trott

@TrottTrott commented Jun 13, 2017

Copy link
Copy Markdown
Member

Update ESLint and configuration to version 4.0.0.

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

tools

@TrottTrott added the tools Issues and PRs related to the tools directory. label Jun 13, 2017
@Trott
Trottforce-pushed the eslint-v4.0.0 branch 2 times, most recently from d0e42e3 to 0069a4dCompareJune 13, 2017 03:56
@Trott

Trott commented Jun 13, 2017•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

This will give us access to more strict indentation linting as well as several new rules (such as no-buffer-constructor hooray hooray!).

sam-github
sam-github reviewed Jun 13, 2017
View reviewed changes

@sam-githubsam-github left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please remove the tools/eslint/node_modules/.bin/eslint dangling symlink

@Trott

Trott commented Jun 13, 2017

Copy link
Copy Markdown
MemberAuthor

please remove the tools/eslint/node_modules/.bin/eslint dangling symlink

Every. Single. Time.

not-an-aardvark
not-an-aardvark approved these changes Jun 13, 2017
View reviewed changes

@not-an-aardvarknot-an-aardvark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly-rubberstamp LGTM

@vsemozhetbyt

vsemozhetbyt commented Jun 13, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Linter CI: https://ci.nodejs.org/job/node-test-linter/9802/

(is it executed with the new version?).

@vsemozhetbyt

vsemozhetbyt commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

Linter CI was suspiciously quick. Is "Linting is not available through the source tarball" message OK?

@targos

targos commented Jun 13, 2017

Copy link
Copy Markdown
Member

That doesn't look right.

cjihrig
cjihrig reviewed Jun 13, 2017
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rubberstamp LGTM

@gibfahn

gibfahn commented Jun 13, 2017•
edited
Loading

Copy link
Copy Markdown
Member

The Makefile checks for tools/eslint/lib/eslint.js. That exists on master but not in this PR.

lib/eslint.js was renamed to lib/linter.js in eslint/eslint@3ec436e. The Makefile and vcbuild.bat will have to be updated. It looks like tools/jslint.js will still work though.

@vsemozhetbyt

vsemozhetbyt commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

Should not the Linter CI fail in such cases?

@gibfahngibfahn mentioned this pull request Jun 13, 2017
Closed
2 tasks
@gibfahn

gibfahn commented Jun 13, 2017

Copy link
Copy Markdown
Member

Should not the Linter CI fail in such cases?

Yes, #13658

gibfahn added a commit to gibfahn/node that referenced this pull request Jun 13, 2017
@gibfahn
build: fail linter if linting not available
2f141ca
Refs: nodejs#13645 (comment)
@jasnell

jasnell commented Jun 13, 2017

Copy link
Copy Markdown
Member

Rubber stamp LGTM

@silverwind

silverwind commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

Can you split into two commits? One for the config changes and one for the update? Not that I care much, but this is how we've been doing it in the past.

@Trott

Trott commented Jun 13, 2017

Copy link
Copy Markdown
MemberAuthor

Can you split into two commits? One for the config changes and one for the update? Not that I care much, but this is how we've been doing it in the past.

Not in this case because then one of the commits will be broken. The config changes are necessary for this version of ESLint. They need to happen in the same commit.

@silverwind

silverwind commented Jun 13, 2017

Copy link
Copy Markdown
Contributor

Not in this case because then one of the commits will be broken

Right, this is a special case with the major version, so it's okay.

refack
refack approved these changes Jun 13, 2017
View reviewed changes

@refackrefack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Works on windows 👍
Rubber stamp code

@TrottTrott added blocked PRs that are blocked by other issues or PRs. and removed blocked PRs that are blocked by other issues or PRs. labels Jun 15, 2017
@Trott

Trott commented Jun 15, 2017

Copy link
Copy Markdown
MemberAuthor

Removed dangling symlink.

Trott added 2 commits June 15, 2017 13:55
@Trott
build: check for linter in bin rather than lib
8eaf72c
Make the "can we lint?" check in Makefile and vcbuild.bat depend on
bin/eslint.js rather than lib/eslint.js. In ESLint 4.0.0, lib/eslint.js
is not present. The lint rules call bin/eslint.js so check for that
instead.
@Trott
tools: update ESLint to v4.0.0
ad0702c
Update ESLint and configuration to version 4.0.0.
addaleax pushed a commit that referenced this pull request Jun 18, 2017
@Trott@addaleax
tools: update ESLint to v4.0.0
8599b91
Update ESLint and configuration to version 4.0.0.
PR-URL: #13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refackrefack mentioned this pull request Jun 19, 2017
Closed
2 tasks
addaleax pushed a commit that referenced this pull request Jun 20, 2017
@Trott@addaleax
build: check for linter in bin rather than lib
2bd73a7
Make the "can we lint?" check in Makefile and vcbuild.bat depend on
bin/eslint.js rather than lib/eslint.js. In ESLint 4.0.0, lib/eslint.js
is not present. The lint rules call bin/eslint.js so check for that
instead.
PR-URL: #13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 20, 2017
@Trott@addaleax
tools: update ESLint to v4.0.0
6697132
Update ESLint and configuration to version 4.0.0.
PR-URL: #13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleaxaddaleax mentioned this pull request Jun 21, 2017
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@gibfahn@addaleax
build: fail linter if linting not available
18f073f
PR-URL: #13658
Ref: #13645 (comment)
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@Trott@addaleax
build: check for linter in bin rather than lib
2ce236e
Make the "can we lint?" check in Makefile and vcbuild.bat depend on
bin/eslint.js rather than lib/eslint.js. In ESLint 4.0.0, lib/eslint.js
is not present. The lint rules call bin/eslint.js so check for that
instead.
PR-URL: #13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@Trott@addaleax
tools: update ESLint to v4.0.0
ff568d4
Update ESLint and configuration to version 4.0.0.
PR-URL: #13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleaxaddaleax mentioned this pull request Jun 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
@gibfahn@MylesBorins
build: fail linter if linting not available
f571868
PR-URL: #13658
Ref: #13645 (comment)
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins

MylesBorins commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

Trott added a commit to Trott/io.js that referenced this pull request Jul 18, 2017
@Trott
build: check for linter in bin rather than lib
6ad4b5e
Make the "can we lint?" check in Makefile and vcbuild.bat depend on
bin/eslint.js rather than lib/eslint.js. In ESLint 4.0.0, lib/eslint.js
is not present. The lint rules call bin/eslint.js so check for that
instead.
PR-URL: nodejs#13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

Trott commented Jul 18, 2017

Copy link
Copy Markdown
MemberAuthor

@MylesBorins Backported in #14340

Trott added a commit to Trott/io.js that referenced this pull request Jul 18, 2017
@Trott
tools: update ESLint to v4.0.0
560472f
Update ESLint and configuration to version 4.0.0.
PR-URL: nodejs#13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 18, 2017
@Trott@MylesBorins
build: check for linter in bin rather than lib
bb1b06a
Make the "can we lint?" check in Makefile and vcbuild.bat depend on
bin/eslint.js rather than lib/eslint.js. In ESLint 4.0.0, lib/eslint.js
is not present. The lint rules call bin/eslint.js so check for that
instead.
Backport-PR-URL: #14340
PR-URL: #13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 18, 2017
@Trott@MylesBorins
tools: update ESLint to v4.0.0
4ffe804
Update ESLint and configuration to version 4.0.0.
Backport-PR-URL: #14340
PR-URL: #13645
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
Merged
@Trott
Trott deleted the eslint-v4.0.0 branch October 10, 2021 01:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrigcjihrigcjihrig approved these changes
+4 more reviewers
@refackrefackrefack approved these changes
@sam-githubsam-githubsam-github approved these changes
@aqrlnaqrlnaqrln approved these changes
@not-an-aardvarknot-an-aardvarknot-an-aardvark 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.

12 participants

@Trott@vsemozhetbyt@targos@gibfahn@jasnell@silverwind@MylesBorins@sam-github@refack@cjihrig@aqrln@not-an-aardvark