Skip to content

tools: update to ESLint 4.8.0#16199

Closed
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:patch-eslint-upgrade
Closed

tools: update to ESLint 4.8.0#16199
apapirovski wants to merge 2 commits into
nodejs:masterfrom
apapirovski:patch-eslint-upgrade

Conversation

@apapirovski
Copy link
Copy Markdown
Contributor

The first commit addresses an extraneous space in lib/modules.js which seems to go unnoticed in v4.3.0 of eslint but gets flagged by v4.8.0. The second commit upgrades to eslint@4.8.0. This mimics the PRs for 4.2.0 & 4.3.0 upgrades.

Here's the changelog: https://github.com/eslint/eslint/blob/master/CHANGELOG.md (Quite a few bug fixes & also some perf changes so I figured it was worth upgrading)

/cc @Trott (since you did the 4.2.0 & 4.3.0 updates)

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

lib, tools

An extra space was not caught by the linter due to what appears
to be a bug in eslint 4.3.0 — remove it.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 14, 2017
Trott
Trott previously requested changes Oct 14, 2017
Copy link
Copy Markdown
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.

This changes about twice as many files as I would expect. Was ESLint updated using tools/update-eslint.sh? If not, can you remove the commit that updates ESLint and use that instead? (And yes, that's probably not documented anywhere and should be.)

@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski commented Oct 14, 2017

@Trott fixed now. Thanks!

(This will teach me not to make a PR jet-lagged on a Friday night. 😂 I even knew that script was there.)

@Trott Trott dismissed their stale review October 14, 2017 21:28

Still seems to update about 70 more files than I get when I do it locally, but I suppose that might be due to different versions of npm or something like that...

@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski commented Oct 14, 2017

@Trott I get 497 (16 more) now but eslint just released 4.9.0 so that's probably the reason for the difference. I'm using npm@latest & dmn@latest. Could be a difference in dmn version maybe? I could see that making a difference...

@not-an-aardvark
Copy link
Copy Markdown
Contributor

The difference in changed-file count might be due to updated package.json metadata, which would change depending on the absolute path where a module is installed.

@apapirovski
Copy link
Copy Markdown
Contributor Author

apapirovski added a commit to apapirovski/node that referenced this pull request Oct 19, 2017
An extra space was not caught by the linter due to what appears
to be a bug in eslint 4.3.0 — remove it.

PR-URL: nodejs#16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
apapirovski added a commit to apapirovski/node that referenced this pull request Oct 19, 2017
PR-URL: nodejs#16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@apapirovski
Copy link
Copy Markdown
Contributor Author

Landed in 10ef563 and 7babce9

@apapirovski apapirovski deleted the patch-eslint-upgrade branch October 19, 2017 13:04
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
An extra space was not caught by the linter due to what appears
to be a bug in eslint 4.3.0 — remove it.

PR-URL: #16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
PR-URL: #16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
An extra space was not caught by the linter due to what appears
to be a bug in eslint 4.3.0 — remove it.

PR-URL: nodejs/node#16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
PR-URL: nodejs/node#16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Copy Markdown
Contributor

This is going to need to be backported to v6.x

Most likely it will make sense to simply update the 6.x branch directly and label this one don't land

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
An extra space was not caught by the linter due to what appears
to be a bug in eslint 4.3.0 — remove it.

PR-URL: nodejs/node#16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#16199
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants