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

Chore: refactor code #12113

Merged
merged 7 commits into from Aug 30, 2019
Merged

Chore: refactor code #12113

merged 7 commits into from Aug 30, 2019

Conversation

@jamesgeorge007
Copy link
Contributor

jamesgeorge007 commented Aug 18, 2019

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: code refactoring

What changes did you make? (Give an overview)
code refactoring

Is there anything you'd like reviewers to focus on?
Nope

@eslint eslint bot added the triage label Aug 18, 2019
Copy link
Member

kaicataldo left a comment

Thanks for contributing! I have one small comment for clarity, but otherwise this LGTM.

lib/init/npm-utils.js Outdated Show resolved Hide resolved
@kaicataldo kaicataldo added chore and removed triage labels Aug 18, 2019
@jamesgeorge007 jamesgeorge007 force-pushed the jamesgeorge007:feat/refactor branch 2 times, most recently from 9d1dff1 to 5712bdb Aug 19, 2019
@jamesgeorge007 jamesgeorge007 force-pushed the jamesgeorge007:feat/refactor branch from 5712bdb to 7cddd55 Aug 19, 2019
lib/init/npm-utils.js Outdated Show resolved Hide resolved
@jamesgeorge007 jamesgeorge007 force-pushed the jamesgeorge007:feat/refactor branch from da67d0b to 771ffab Aug 19, 2019
@jamesgeorge007

This comment has been minimized.

Copy link
Contributor Author

jamesgeorge007 commented Aug 19, 2019

lib/init/npm-utils.js Outdated Show resolved Hide resolved
have deps as a Set rather than an array
Copy link
Member

platinumazure left a comment

One more small tweak. Please also think about whether a test might be needed, related to the issue I've flagged in this review. Thanks for your continued efforts!

lib/init/npm-utils.js Outdated Show resolved Hide resolved
@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Aug 20, 2019

It does appear this isn’t tested, since tests were passing in the last iteration. It would be great if we could cover it with a test case.

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Aug 24, 2019

@jamesgeorge007 Looks like tests are failing. Do you mind fixing them up?

@aladdin-add aladdin-add self-requested a review Aug 28, 2019
Copy link
Member

aladdin-add left a comment

LGTM, thanks!

@eslint eslint deleted a comment from tacoto12 Aug 29, 2019
Copy link
Member

platinumazure left a comment

LGTM, thanks!

@tacoto12

This comment has been minimized.

Copy link

tacoto12 commented Aug 29, 2019

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Aug 29, 2019

@tacoto12 That wasn't addressed to you.

It looks like you've somehow been subscribed to this thread and other threads through GitHub. Please sign in on GitHub and unsubscribe. Commenting/replying won't help in your case as you will just continue to get notifications.

If you have questions, please reach out to GitHub Support. Thanks!

Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@kaicataldo kaicataldo merged commit 0acdefb into eslint:master Aug 30, 2019
9 checks passed
9 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20190824.5 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@jamesgeorge007 jamesgeorge007 deleted the jamesgeorge007:feat/refactor branch Aug 31, 2019
@eslint eslint bot locked and limited conversation to collaborators Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.