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

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-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Aug 18, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

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 This change is not user-facing and removed triage An ESLint team member will look at this issue soon labels Aug 18, 2019
@jamesgeorge007 jamesgeorge007 force-pushed the feat/refactor branch 2 times, most recently from 9d1dff1 to 5712bdb Compare August 19, 2019 01:28
lib/init/npm-utils.js Outdated Show resolved Hide resolved
@jamesgeorge007
Copy link
Contributor Author

cc @kaicataldo @g-plane

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

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

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

@aladdin-add aladdin-add self-requested a review August 28, 2019 13:35
Copy link
Member

@aladdin-add aladdin-add 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!

Copy link
Member

@platinumazure platinumazure 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!

@tacoto12
Copy link

tacoto12 commented Aug 29, 2019 via email

@platinumazure
Copy link
Member

@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 kaicataldo 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!

@kaicataldo kaicataldo merged commit 0acdefb into eslint:master Aug 30, 2019
@jamesgeorge007 jamesgeorge007 deleted the feat/refactor branch August 31, 2019 01:38
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 27, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants