Skip to content

chore: refactor code to eliminate redundancy#1002

Merged
evenstensberg merged 10 commits into
webpack:nextfrom
jamesgeorge007:feat/refactor
Aug 13, 2019
Merged

chore: refactor code to eliminate redundancy#1002
evenstensberg merged 10 commits into
webpack:nextfrom
jamesgeorge007:feat/refactor

Conversation

@jamesgeorge007

@jamesgeorge007 jamesgeorge007 commented Jul 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

refactoring

Did you add tests for your changes?
Nope

If relevant, did you update the documentation?
N/A
Summary
Made a minor refactor so as to eliminate redundancy.

N/A
Does this PR introduce a breaking change?

Nope
Other information
Follow up of #972

@jamesgeorge007 jamesgeorge007 mentioned this pull request Jul 17, 2019
Closed
evenstensberg
evenstensberg requested changes Jul 30, 2019
View reviewed changes

@evenstensberg evenstensberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this code was recently refactored so that the src would be more compact. Is there any reason for changing that?

Comment thread packages/utils/__tests__/package-manager.test.ts Outdated
@webpack-bot

webpack-bot commented Jul 31, 2019

Copy link
Copy Markdown

@jamesgeorge007 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@evenstensberg Please review the new changes.

@evenstensberg

evenstensberg commented Aug 4, 2019

Copy link
Copy Markdown
Member

I mean that we refactored this recently, that's why the code is like it is. Is there any reason for changing this?

@jamesgeorge007

jamesgeorge007 commented Aug 4, 2019

Copy link
Copy Markdown
Contributor Author

The respective files weren't touched in the previous refactor 🤔

@jamesgeorge007

jamesgeorge007 commented Aug 4, 2019

Copy link
Copy Markdown
Contributor Author

As per this comment

ematipico
ematipico previously approved these changes Aug 5, 2019
View reviewed changes
rishabh3112
rishabh3112 previously approved these changes Aug 5, 2019
View reviewed changes
evenstensberg
evenstensberg reviewed Aug 11, 2019
View reviewed changes
Comment thread packages/utils/__tests__/package-manager.test.ts Outdated
@jamesgeorge007
jamesgeorge007 dismissed stale reviews from rishabh3112 and ematipico via b51df6a August 12, 2019 05:06
@jamesgeorge007
jamesgeorge007 requested a review from a team as a code owner August 12, 2019 05:06
@jamesgeorge007

jamesgeorge007 commented Aug 12, 2019

Copy link
Copy Markdown
Contributor Author

@evenstensberg as you may have noticed, the respective test case for yarn is to be revisited (out of scope of this PR). The only possible workaround is to ignore (comment) or drop it.

@webpack-bot

webpack-bot commented Aug 12, 2019

Copy link
Copy Markdown

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@evenstensberg
evenstensberg merged commit e45e703 into webpack:next Aug 13, 2019
@jamesgeorge007
jamesgeorge007 deleted the feat/refactor branch August 13, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evenstensberg evenstensberg evenstensberg approved these changes
+3 more reviewers
@ematipico ematipico ematipico approved these changes
@dhruvdutt dhruvdutt dhruvdutt approved these changes
@rishabh3112 rishabh3112 rishabh3112 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR: CI-ok PR: reviewed-approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jamesgeorge007 @webpack-bot @evenstensberg @ematipico @dhruvdutt @rishabh3112