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

chore: fix and refactor add generator #843

Closed
wants to merge 17 commits into from
Closed

chore: fix and refactor add generator #843

wants to merge 17 commits into from

Conversation

rishabh3112
Copy link
Member

What kind of change does this PR introduce?

refactoring

Did you add tests for your changes?
No

If relevant, did you update the documentation?
N/A

Summary
Updating prompting() to use async/await

Does this PR introduce a breaking change?

Maybe

Other information
#836

@rishabh3112 rishabh3112 changed the title chore(async): add generator to async/await [WIP] chore(async): add generator to async/await Apr 21, 2019
@rishabh3112 rishabh3112 marked this pull request as ready for review April 21, 2019 04:39
pranshuchittora
pranshuchittora reviewed Apr 21, 2019
View reviewed changes
Copy link
Member

@pranshuchittora pranshuchittora left a comment •
edited
Loading

Choose a reason for hiding this comment

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

IMO this might affect node 6.x users.

Node supports async/await out of the box since version 7.6.

@rishabh3112
Copy link
Member Author

Refer #836

@pranshuchittora
Copy link
Member

@rishabh3112 👍

@anshumanv anshumanv mentioned this pull request Apr 22, 2019
Closed
8 tasks
@rishabh3112 rishabh3112 changed the title [WIP] chore(async): add generator to async/await chore(async): add generator to async/await Apr 22, 2019
@rishabh3112
Copy link
Member Author

Ready for a review now 👍

evenstensberg
evenstensberg reviewed Apr 22, 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'll do some manual testing while I got time, but nice work.

packages/generators/add-generator.ts Outdated
}
}

const temp: string = action;
Copy link
Member

Choose a reason for hiding this comment

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

reassigning a const

Copy link
Member Author

Choose a reason for hiding this comment

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

Where? temp is never reassigned anywhere and here it is declared.

ematipico
ematipico suggested changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Left some feedback!

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

Thanks for review @ematipico
I will look into it and resolve these in this PR.

P.S
Just for clearing things 😅,
All of the code, in which you requested changes, was there as it is before this PR and not written by me originally.

@ematipico
Copy link
Contributor

ematipico commented Apr 23, 2019 •
edited
Loading

Oh yeah don't worry! We know it's working all right, if you think it doesn't require any change and it will steal too much time of yours, mark it as resolved and move along. :)

@rishabh3112
Copy link
Member Author

Well all are done and it didn't steal my time :). Pushing commits after a quick test.

@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

Done @evenstensberg

@rishabh3112
Copy link
Member Author

@webpack/cli-team can I have a review on this PR, this is fine IMO

@rishabh3112 rishabh3112 mentioned this pull request May 24, 2019
Merged
@rishabh3112
Copy link
Member Author

cc @evenstensberg

anshumanv
anshumanv approved these changes May 24, 2019
View reviewed changes
Copy link
Member

@anshumanv anshumanv left a comment

Choose a reason for hiding this comment

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

Seems good to go!

@rishabh3112
Copy link
Member Author

@evenstensberg PTAL 😅

@evenstensberg
Copy link
Member

Thanks, will get back to you this week. Quite a big PR and need to manually test

evenstensberg
evenstensberg reviewed May 28, 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.

Triaging #817 first to avoid complication of work

@evenstensberg
Copy link
Member

@rishabh3112 could you rebase and let's continue on getting this merged once you're done?

@rishabh3112
Copy link
Member Author

Sure, will catch you tommorow. It's night here now and I am sleeping :)

@evenstensberg
Copy link
Member

Gotcha, sleep tight!

@webpack-bot
Copy link

@rishabh3112 Thanks for your update.

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

@anshumanv Please review the new changes.

evenstensberg
evenstensberg reviewed May 31, 2019
View reviewed changes
tsconfig.json Outdated Show resolved Hide resolved
@rishabh3112 rishabh3112 mentioned this pull request Jun 3, 2019
Merged
@evenstensberg
Copy link
Member

Could you rebase?

@rishabh3112
Copy link
Member Author

Will do it @evenstensberg, but is it required now?, as add package is being removed Or we would keep generators and remove packages only?

@evenstensberg
Copy link
Member

Nah I don't think it's worth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evenstensberg evenstensberg evenstensberg left review comments

@pranshuchittora pranshuchittora pranshuchittora left review comments

@dhruvdutt dhruvdutt dhruvdutt requested changes

@ematipico ematipico ematipico approved these changes

@anshumanv anshumanv anshumanv approved these changes

Assignees
No one assigned
Labels
PR: CI-ok PR: review-outdated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@rishabh3112 @pranshuchittora @ematipico @webpack-bot @evenstensberg @dhruvdutt @anshumanv