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

Breaking: Upgrade yeoman dependencies to latest #101

Merged
merged 3 commits into from Jul 22, 2021

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented Jul 13, 2021

Significantly modernizes code and tests.
Significantly improves test coverage.

TODO:

  • Substantial test rewrite
  • Chore: Fix CI to run on correct branch #102 should be merged first
  • Dependency installation issues:
    - Uncaught Error: Cannot find module 'rimraf'
    - Uncaught Error: Cannot find module 'mkdirp'
    - Recreating package-lock.json helped, but I was still seeing errors on Node 12/14 until I switched CI to use NPM v7 when installing dependencies. This is the same thing the yeoman-environment does on its CI configuration.

Fixes #43.
Fixes #86.

CC: @platinumazure

@bmish bmish marked this pull request as ready for review July 14, 2021 23:17
@bmish bmish marked this pull request as draft July 14, 2021 23:17
@bmish bmish force-pushed the yeoman-upgrade branch 10 times, most recently from 5853fd6 to e8591e7 Compare July 15, 2021 05:52
@bmish bmish marked this pull request as ready for review July 15, 2021 05:56
@bmish
Copy link
Sponsor Member Author

bmish commented Jul 15, 2021

Ready for review.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just a few minor comments.

tests/app/index.js Outdated Show resolved Hide resolved
it("should run the eslint:rule generator", function() {
assert.ok(this.spy.calledOnce);
it("does not blow up", () => {
assert.ok(true);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this test isn’t doing anything. Maybe a placeholder you meant to update?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Added real test for this.

tests/plugin/index.js Outdated Show resolved Hide resolved
tests/rule/index.js Outdated Show resolved Hide resolved
Significantly modernizes code and tests.
Significantly improves test coverage.
With the latest yeoman dependencies,  I was seeing errors under Node 12/14, unless NPM 7 was used to install dependencies:

- Uncaught Error: Cannot find module 'rimraf'
- Uncaught Error: Cannot find module 'mkdirp'
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@nzakas nzakas merged commit d5c7a80 into eslint:main Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade dependencies Upgrade yeoman-generator to latest
2 participants