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.

Sign up for GitHub

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

Jump to bottom

tools: update eslint to latest #28173

Closed
wants to merge 4 commits into from
Closed

tools: update eslint to latest #28173

wants to merge 4 commits into from

Conversation

BridgeAR
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Sadly, an error occurred when I tried to trigger a build. :(

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 11, 2019
targos
targos approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

RSLGTM

trivikr
trivikr approved these changes Jun 11, 2019
View reviewed changes
Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

RSLGTM

@BridgeAR
Copy link
Member Author

Lite CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3674/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 11, 2019
@Trott
Copy link
Member

Trott commented Jun 12, 2019 •
edited
Loading

Nit 1: Please specify what version you are upgrading to in the commit message. (Preferably what version we're upgrading from as well.)

Nit 2: If I'm not mistaken, unneeded disable comments will result in errors with our configuration. If I'm right about that, then make lint-js will fail when the first commit lands but the second does not. If that's the case (and again, I believe it is), please squash the two commits into a single commit so that there isn't a commit where linting fails.

@BridgeAR
Copy link
Member Author

@Trott thanks, I forgot that it would complain about the comments if they are not required. I updated the PR and incorporated the suggestions.

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3675/

@BridgeAR
tools: update eslint
0f116e5 
This updates eslint from v6.0.0-alpha.2 to v6.0.0-rc.0.

This also removes eslint-disable comments about `bigint` typeof
checks. Those would otherwise have caused linting errors now that
`bigint` is accepted as valid entry.
@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3686/

@BridgeAR
Copy link
Member Author

New CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3694/

@ZYSzys
Copy link
Member

ZYSzys commented Jun 22, 2019 •
edited
Loading

@BridgeAR eslint@6.0.0 was just released. How about updating to v6.0.0 directly ?

And update this line to eslint@latest ?

node/tools/update-eslint.sh

Line 16 in e4136c1

npm install --global-style --no-bin-links --production --no-package-lock eslint@next

Refs: https://github.com/eslint/eslint/releases/tag/v6.0.0

@BridgeAR
Copy link
Member Author

Updated to the latest release.

ZYSzys
ZYSzys approved these changes Jun 23, 2019
View reviewed changes
Copy link
Member

@ZYSzys ZYSzys left a comment

Choose a reason for hiding this comment

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

Still RSLGTM.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 25, 2019

ESLint 6.0.1 was released. Please skip 6.0.0 before landing this.

@BridgeAR
Copy link
Member Author

Lite CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3721/

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 27, 2019
@BridgeAR
tools: update eslint
ed8fc7e 
This updates eslint from v6.0.0-alpha.2 to v6.0.1

This also removes eslint-disable comments about `bigint` typeof
checks. Those would otherwise have caused linting errors now that
`bigint` is accepted as valid entry.

PR-URL: nodejs#28173
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in ed8fc7e

@BridgeAR BridgeAR closed this Jun 27, 2019
targos pushed a commit that referenced this pull request Jul 2, 2019
@BridgeAR @targos
tools: update eslint
023c75e 
This updates eslint from v6.0.0-alpha.2 to v6.0.1

This also removes eslint-disable comments about `bigint` typeof
checks. Those would otherwise have caused linting errors now that
`bigint` is accepted as valid entry.

PR-URL: #28173
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
targos pushed a commit that referenced this pull request Jul 2, 2019
@BridgeAR @targos
tools: update eslint
b744bd9 
This updates eslint from v6.0.0-alpha.2 to v6.0.1

This also removes eslint-disable comments about `bigint` typeof
checks. Those would otherwise have caused linting errors now that
`bigint` is accepted as valid entry.

PR-URL: #28173
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@targos targos mentioned this pull request Jul 2, 2019
Merged
@beevelop beevelop mentioned this pull request Jul 3, 2019
Closed
This was referenced Jul 3, 2019
Closed
Closed
@BridgeAR BridgeAR deleted the update-eslint-to-latest branch January 20, 2020 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@trivikr trivikr trivikr approved these changes

@targos targos targos approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@BridgeAR @nodejs-github-bot @Trott @ZYSzys @cjihrig @targos @trivikr