Skip to content

fix(add): add handling of merge option #797

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
Merged
evenstensberg merged 38 commits into from
Jun 6, 2019
Merged

fix(add): add handling of merge option #797

evenstensberg merged 38 commits into from
Jun 6, 2019

Conversation

@rishabh3112
Copy link
Member

@rishabh3112 rishabh3112 commented Mar 20, 2019 •
edited by ematipico
Loading

BugFix #795
Closed #795
What kind of change does this PR introduce?

bugfix
Did you add tests for your changes?
N/A
If relevant, did you update the documentation?
N/A
Summary

Does this PR introduce a breaking change?

Other information

ematipico
ematipico suggested changes Mar 20, 2019
View reviewed changes
@rishabh3112
Copy link
Member Author

rishabh3112 commented Mar 20, 2019 •
edited
Loading

Hi @ematipico, I am pushing a sample refactoring
( will break as add-generator is importing non npm-published changes at @webpack-cli/utils).
If this refactoring seems fine to you, I will continue refactoring this and all generators as time permits in seperate PRs. 👍

EDIT: Fixed lint errors @ematipico

@webpack-bot
Copy link

webpack-bot commented Mar 21, 2019

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

@rishabh3112
Copy link
Member Author

rishabh3112 commented Mar 23, 2019

@ematipico Have a review when free 😅.

@rishabh3112
Copy link
Member Author

rishabh3112 commented Mar 23, 2019

CLA bot is showing that I have not signed 🤔. Some issue here @ematipico

@dhruvdutt
Copy link
Contributor

dhruvdutt commented Mar 24, 2019

@rishabh3112 It shows signed for me. All checks success.

evenstensberg
evenstensberg reviewed Mar 25, 2019
View reviewed changes
evenstensberg
evenstensberg reviewed Apr 8, 2019
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

I don't have time to do a thorough review for this until later this month so putting this on Disney on Ice

evenstensberg
evenstensberg reviewed May 28, 2019
View reviewed changes
@rishabh3112
Copy link
Member Author

rishabh3112 commented Jun 5, 2019

Have applied suggestions @evenstensberg and left some comments for you :)

evenstensberg
evenstensberg reviewed Jun 5, 2019
View reviewed changes
evenstensberg
evenstensberg requested changes Jun 6, 2019
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

These are the final changes requested before this is good to go, nice job!

evenstensberg
evenstensberg reviewed Jun 6, 2019
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Native modules first, those are those that are already shipped with node like fs, path etc, then the ones like autoprompt, followed up by @scoped/aye

@rishabh3112
Copy link
Member Author

rishabh3112 commented Jun 6, 2019

Have moved local imports (example ../utils) at last.

evenstensberg
evenstensberg requested changes Jun 6, 2019
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

last comments

evenstensberg
evenstensberg approved these changes Jun 6, 2019
View reviewed changes
Copy link
Member

@evenstensberg evenstensberg 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 for your hard work in this 🥇

@rishabh3112
Copy link
Member Author

rishabh3112 commented Jun 6, 2019

No problem, Thanks🎉🎉

@evenstensberg evenstensberg merged commit 98aa6f3 into webpack:master Jun 6, 2019
@rishabh3112 rishabh3112 deleted the fix/add branch December 25, 2019 16:34
@enterstudio enterstudio mentioned this pull request May 11, 2022
Open
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

+2 more reviewers

@ematipico ematipico ematipico requested changes

@wizardofhogwarts wizardofhogwarts wizardofhogwarts approved these changes

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.

[BUG]: @webpack-cli/add not working on topScope and merge

6 participants

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