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 eslint #27670

Closed
wants to merge 8 commits into from
Closed

tools: update eslint #27670

wants to merge 8 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 13, 2019

Eslint v6 is still an alpha but we might want to update to that version nevertheless. That way we profit from recent fixes they implemented and there is probably little downside switching right now.

I also went ahead and activated two more eslint rules that are recommended by eslint. This update does not require any code changes besides changing one builtinGlobals option for no-redeclare (the default changed but we should stick to the old value).

Update: This also includes an update to the latest stable bable-eslint module and removes a lot of files due to an update to dmn.

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

@BridgeAR BridgeAR requested review from Trott and cjihrig May 13, 2019 11:36
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 13, 2019
@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label May 13, 2019
@Trott

This comment has been minimized.

@BridgeAR
Copy link
Member Author

@Trott I am fine to wait but isn't the referenced release another pre-release? I guess there's little downside to updating to the pre-release. I guess the worst that would happen is that we have to update more often?

I wanted to see what would break and what we should update. The regular lint job passes locally with only a single rule adjusted. So that's quite nice. The CI job failed though and it seems like there's an issue around the new configuration. I fixed that locally and hope the CI passes now as well.

@BridgeAR
Copy link
Member Author

@nodejs/build it seems like this would require a manual update to the node-linter workspace? I can't reproduce the CI error locally.

@richardlau
Copy link
Member

@nodejs/build it seems like this would require a manual update to the node-linter workspace? I can't reproduce the CI error locally.

Can you check if you comitted lib/cli-engine which looks to be a newly added directory?

@not-an-aardvark
Copy link
Contributor

I don't have any opinion on this change, but @BridgeAR is correct that the May 24th ESLint release will in all likelihood be another prerelease, not the stable v6.0.0 release. (There isn't a set timeline for when the stable release will happen, but my best estimate would be sometime in June.)

@BridgeAR
Copy link
Member Author

@richardlau

Can you check if you comitted lib/cli-engine which looks to be a newly added directory?

It's all here. I used the update script and running the make lint-js and make lint-js-ci both pass locally but they fail on the CI. Since this fails on both, Travis and our CI, it has to be something in the code... I just can't yet find the reason why it fails.

@richardlau

This comment has been minimized.

@BridgeAR
Copy link
Member Author

@richardlau thanks, I saw the filename, not the folder and did not look further.

It turned out that we ignored quite a lot of files that we should not have ignored. We probably still ignore doc linting files but I wanted to concentrate on this specific change for now. @refack PTAL

@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label May 14, 2019
@BridgeAR
Copy link
Member Author

dmn did not clean all files as it should. I opened a PR to fix that: inikulin/dmn#27

@BridgeAR
Copy link
Member Author

BridgeAR commented May 14, 2019

This removes a lot of files now. The reason for that is that I used the new dmn version to remove old files.

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

@Trott does this seem fine for you to land? I would otherwise split some parts into a different PR.

@BridgeAR BridgeAR changed the title tools: update eslint to v6.0.0-alpha.1 tools: update eslint May 14, 2019
@Trott
Copy link
Member

Trott commented May 14, 2019

@Trott does this seem fine for you to land? I would otherwise split some parts into a different PR.

No opinion from me. I'm fine if this lands, but I'm also fine with waiting until the stable release.

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels May 14, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented Jun 4, 2019

Rebased due to conflicts.

@nodejs-github-bot
Copy link
Collaborator

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
This makes sure the "good" and "bad" examples are split into two.
Otherwise it'll result in an parse error.

PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
This makes sure the eslint config is up to date for eslint 6.0.0.

PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
This activates the following recommended eslint rules:

- no-async-promise-executor
- no-shadow-restricted-names

PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
This is just a minor update to use the newest latest version.

PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
This updates out individual linting to work with eslint v6. Without
this change the node_modules would also be checked for.

PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
This increases the maximum number of files to lint per worker from
40 to 60 files. This should ideally reduce the total linting time
a tiny bit.

PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 11, 2019
The `-f` flag was missing a space after the module version.

PR-URL: nodejs#27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in fddc2d7...0cd112a 🎉

@BridgeAR BridgeAR closed this Jun 11, 2019
BridgeAR added a commit that referenced this pull request Jun 17, 2019
This makes sure the "good" and "bad" examples are split into two.
Otherwise it'll result in an parse error.

PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
This makes sure the eslint config is up to date for eslint 6.0.0.

PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
This activates the following recommended eslint rules:

- no-async-promise-executor
- no-shadow-restricted-names

PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
This is just a minor update to use the newest latest version.

PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
This updates out individual linting to work with eslint v6. Without
this change the node_modules would also be checked for.

PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
This increases the maximum number of files to lint per worker from
40 to 60 files. This should ideally reduce the total linting time
a tiny bit.

PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
BridgeAR added a commit that referenced this pull request Jun 17, 2019
The `-f` flag was missing a space after the module version.

PR-URL: #27670
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
@BridgeAR BridgeAR deleted the update-eslint branch January 20, 2020 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants