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.

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

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

Merged
merged 38 commits into from Jun 6, 2019
Merged

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

merged 38 commits into from Jun 6, 2019

Conversation

rishabh3112
Copy link
Member

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

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
packages/generators/add-generator.ts Outdated Show resolved Hide resolved
packages/generators/add-generator.ts Outdated Show resolved Hide resolved
@rishabh3112
Copy link
Member Author

rishabh3112 commented Mar 20, 2019 •
edited

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

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

@rishabh3112
Copy link
Member Author

@ematipico Have a review when free 😅.

@rishabh3112
Copy link
Member Author

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

@dhruvdutt
Copy link
Member

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

evenstensberg
evenstensberg reviewed Mar 25, 2019
View reviewed changes
packages/utils/generators/add/questions/index.ts Outdated Show resolved Hide resolved
packages/utils/generators/add/questions/index.ts Outdated Show resolved Hide resolved
packages/generators/add-generator.ts Outdated Show resolved Hide resolved
packages/generators/add-generator.ts Outdated Show resolved Hide resolved
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
packages/utils/package.json Outdated Show resolved Hide resolved
packages/utils/scaffold.ts Outdated Show resolved Hide resolved
@rishabh3112
Copy link
Member Author

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

evenstensberg
evenstensberg reviewed Jun 5, 2019
View reviewed changes
packages/generators/add-generator.ts Outdated Show resolved Hide resolved
packages/generators/add-generator.ts Outdated Show resolved Hide resolved
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!

packages/generators/add-generator.ts Outdated Show resolved Hide resolved
packages/generators/utils/add/questions.ts Show resolved Hide resolved
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

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

packages/generators/add-generator.ts Outdated Show resolved Hide resolved
packages/generators/utils/add/questions.ts Show resolved Hide resolved
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

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

@ematipico ematipico ematipico requested changes

@evenstensberg evenstensberg evenstensberg approved these changes

@wizardofhogwarts wizardofhogwarts wizardofhogwarts approved these changes

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