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

fix(init): refactored the init.ts success message #810

Merged
merged 2 commits into from Apr 1, 2019

Conversation

pranshuchittora
Copy link
Member

Previously there was multiple stdout.write due to the presence of conditional statements. In this PR I have scrapped those repeating lines into a single process.stdout.write

What kind of change does this PR introduce?
Refactoring

Did you add tests for your changes?
No

If relevant, did you update the documentation?
Can't say

Does this PR introduce a breaking change?
Probably not

now the success message is more maintainable
@webpack-bot
Copy link

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

packages/init/init.ts Outdated Show resolved Hide resolved
refactored init.ts to make code more readable
@webpack-bot
Copy link

@pranshuchittora Thanks for your update.

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

@ematipico Please review the new changes.

Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

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

LGTM

packages/init/init.ts Show resolved Hide resolved
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.

Nice refactor!

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.

Do you have a screenshot of the fix?

@pranshuchittora
Copy link
Member Author

@evenstensberg screenshot of changes ?

@evenstensberg
Copy link
Member

Take a screenshot of the change

@pranshuchittora
Copy link
Member Author

pranshuchittora commented Mar 31, 2019

@evenstensberg screenshot captured 👍

Screenshot from 2019-03-31 19-24-10

@evenstensberg
Copy link
Member

The visual change, not code

@pranshuchittora
Copy link
Member Author

No change as such.
In order to check for visual differences, I have added 🎉 in the string for testing purposes.

Before
Screenshot from 2019-04-01 19-20-51

After
Screenshot from 2019-04-01 19-21-10
(The emoji is just for testing, its not been included in the PR)

Hence,this PR doesn't introduces and visual change.

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

@ematipico ematipico merged commit 6cfed0f into webpack:master Apr 1, 2019
misterdev pushed a commit to misterdev/webpack-cli that referenced this pull request Apr 23, 2019
Previously there was multiple `stdout.write` due to the presence of conditional statements. In this PR I have scrapped those repeating lines into a single `process.stdout.write`

<!-- Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. -->

**What kind of change does this PR introduce?**
Refactoring

**Did you add tests for your changes?**
No

**If relevant, did you update the documentation?**
Can't say

**Does this PR introduce a breaking change?**
Probably not
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants