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 5.0.0 #20855

Merged
merged 2 commits into from Jun 24, 2018
Merged

tools: update ESLint to 5.0.0 #20855

merged 2 commits into from Jun 24, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 20, 2018 •
edited

This is still a work in progress. ESLint 5.x hasn't actually been released yet. This PR contains ESLint v5.0.0 v5.0.0-alpha.3 v5.0.0-rc.0. The two commits also need to be rebased in the opposite order to avoid a broken commit from being in master.

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

@cjihrig cjihrig requested a review from a team as a code owner May 20, 2018 19:30
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label May 20, 2018
@BridgeAR
Copy link
Member

I would rather wait until this is not a pre release anymore. Especially since the fixes that this currently provides do not seem to be really important.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 20, 2018

That's why it's listed as WIP

@BridgeAR
Copy link
Member

But in that case this is going to be open for likely quite a while until it could land?

@cjihrig
Copy link
Contributor Author

cjihrig commented May 20, 2018

Possibly. But that's not a big deal.

@BridgeAR
Copy link
Member

I disagree. I think it is bad to have a PR open that could just be opened when the PR could also land. Long standing open PRs are overhead for other collaborators as they will look into them from time to time. So -1 on that.

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label May 20, 2018
@addaleax
Copy link
Member

That overhead is precisely why we have the “blocked” label. :)

If doing it this was is easier for somebody, that’s very much fine with me. Let’s not prescribe workflows when we don’t have to.

@nodejs nodejs deleted a comment from refack Jun 1, 2018
@Trott Trott removed the request for review from a team June 1, 2018 22:28
@Trott
Copy link
Member

Trott commented Jun 1, 2018

@cjihrig You may already be aware, but alpha 4 is out if you want to update. (Not sure if you want to track closely in this PR or if you'd rather only push to the branch when the ESLint update results in other code changes in our code base.)

@Trott
Copy link
Member

Trott commented Jun 22, 2018

ESLint 5.0.0 should be out in a few hours based on eslint/eslint#10458.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 22, 2018

I updated this PR to 5.0.0-rc.0 the other night. I've also been following along with the ESLint release issues. My question is, where does it specify the version of the upcoming release? For example, will it be 5.0.0, 5.0.0-rc.1, or something completely different?

@Trott
Copy link
Member

Trott commented Jun 22, 2018

@cjihrig Oh, good point, it very well could be rc1. I was making assumptions...

@Trott
Copy link
Member

Trott commented Jun 22, 2018

@cjihrig I asked on the ESLint Gitter channel and @not-an-aardvark replied:

It will be 5.0.0, although the timing isn't entirely clear yet because we're still figuring out who is available to do the release. There's a possibility that it will instead happen tomorrow or sometime later this weekend.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 22, 2018

Thanks, I'll be on the lookout for the release (not that we are in a huge rush for it).

@not-an-aardvark would it be possible for the ESLint team to start including the upcoming version in related GitHub issues? If there is a good reason not to, that's fine, but it would be helpful to not have to follow Gitter as well.

@not-an-aardvark
Copy link
Contributor

I'd be open to adding more information there, That said, the decision about what type of release to create (minor vs patch, when we're not doing prereleases) is usually determined based on the commits that have landed since the previous release, so it might change shortly before a release happens if a semver-minor PR is merged. Is the confusion mainly about scheduling for prereleases/first stable releases?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 23, 2018

Is the confusion mainly about scheduling for prereleases/first stable releases?

Yea, I think that's it. As an outsider that doesn't follow everything on the eslint repo, I don't have much insight into that. For example, it looks like v5 is going to have 5 alphas and 1 rc, while v4 had 3 alphas, 1 beta, and 1 rc. If those decisions are known ahead of time, it would be great to have in the issue tracker.

@not-an-aardvark
Copy link
Contributor

Unfortunately the release schedule for major releases isn't decided very far in advance, and is largely based on external factors (e.g. how many bug reports we get). The decision that today's release would be stable was only made about 2 weeks ago. That said, for future releases I'll try to keep the issues more up to date with when the decisions are made.

@not-an-aardvark
Copy link
Contributor

ESLint v5.0.0 has been released: https://eslint.org/blog/2018/06/eslint-v5.0.0-released

@cjihrig cjihrig changed the title WIP: tools: update ESLint to 5.0.0 tools: update ESLint to 5.0.0 Jun 23, 2018
@cjihrig cjihrig removed the blocked PRs that are blocked by other issues or PRs. label Jun 23, 2018
@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 23, 2018

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

Trott
Trott approved these changes Jun 23, 2018
View reviewed changes
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 if CI is green

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 23, 2018

Looks like there is an issue with ARM infrastructure - https://ci.nodejs.org/job/node-test-binary-arm/1986/.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 24, 2018

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

The results look better. The FreeBSD flake didn't show up, and only one ARM machine had an infrastructure issue, which seems par for the course this weekend.

@cjihrig
test: lint fixes for ESLint update
d5316bc 
This commit introduces changes required for the ESLint 5 update.

PR-URL: nodejs#20855
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig
tools: update ESLint to 5.0.0
7a3bbbf 
This is a new major release of ESLint.

PR-URL: nodejs#20855
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig cjihrig merged commit 7a3bbbf into nodejs:master Jun 24, 2018
@cjihrig cjihrig deleted the eslint branch June 24, 2018 16:47
targos pushed a commit that referenced this pull request Jun 24, 2018
@cjihrig @targos
test: lint fixes for ESLint update
357eaf3 
This commit introduces changes required for the ESLint 5 update.

PR-URL: #20855
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 24, 2018
@cjihrig @targos
tools: update ESLint to 5.0.0
21d73a5 
This is a new major release of ESLint.

PR-URL: #20855
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jul 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@cjihrig @BridgeAR @addaleax @Trott @not-an-aardvark @nodejs-github-bot