Navigation Menu

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

Jump to bottom

fix: respect --color/--no-color arguments #2042

Merged
merged 6 commits into from Nov 9, 2020
Merged

fix: respect --color/--no-color arguments #2042

merged 6 commits into from Nov 9, 2020

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Nov 5, 2020 •
edited

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
no
Summary

stats colors should be disabled if we use --no-color.

Fixes #2038 as well

Before

Screenshot at 2020-11-05 09-11-53

After

Screenshot at 2020-11-05 09-12-28

Does this PR introduce a breaking change?
Nope

Other information
No

@codecov
Copy link

codecov bot commented Nov 5, 2020 •
edited

Codecov Report

Merging #2042 (8a3e10e) into master (cd56712) will increase coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2042      +/-   ##
==========================================
+ Coverage   68.50%   68.65%   +0.14%     
==========================================
  Files          76       76              
  Lines        2413     2418       +5     
  Branches      497      497              
==========================================
+ Hits         1653     1660       +7     
+ Misses        760      758       -2
Impacted Files Coverage Δ
packages/webpack-cli/lib/utils/cli-flags.js 100.00% <ø> (ø)
packages/webpack-cli/lib/bootstrap.js 100.00% <100.00%> (ø)
packages/webpack-cli/lib/webpack-cli.js 90.43% <100.00%> (+0.23%) ⬆️
packages/generate-loader/src/index.ts 100.00% <0.00%> (ø)
packages/utils/src/modify-config-helper.ts 54.38% <0.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd56712...8a3e10e. Read the comment docs.

@snitin315
Copy link
Member Author

snitin315 commented Nov 5, 2020 •
edited

from CI -

webpack-4
webpack-5

@snitin315 snitin315 marked this pull request as ready for review November 5, 2020 04:37
@snitin315 snitin315 requested a review from a team as a code owner November 5, 2020 04:37
alexander-akait
alexander-akait approved these changes Nov 5, 2020
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @webpack/cli-team

Good job, thanks

alexander-akait
alexander-akait previously requested changes Nov 5, 2020
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment •
edited

Choose a reason for hiding this comment

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

oh, what about logger? Because --no-colors should disable colors for logger too

rishabh3112
rishabh3112 previously approved these changes Nov 5, 2020
View reviewed changes
@snitin315
Copy link
Member Author

oh, what about logger? Because --no-colors should disable colors for logger too

I will update 👍🏻

@snitin315
Copy link
Member Author

I will fix the CI problems soon.

This was referenced Nov 7, 2020
Merged
Merged
@snitin315 snitin315 force-pushed the fix/stats-color branch 4 times, most recently from 232dea6 to d30018f Compare November 7, 2020 06:32
@snitin315
Copy link
Member Author

/cc @webpack/cli-team

alexander-akait
alexander-akait approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @webpack/cli-team

@snitin315
Copy link
Member Author

Good to merge?

alexander-akait
alexander-akait previously approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @webpack/cli-team

@alexander-akait alexander-akait mentioned this pull request Nov 9, 2020
Merged
@anshumanv
Copy link
Member

Thanks for waiting, I'm on it 👍

anshumanv
anshumanv requested changes Nov 9, 2020
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.

minor nits

packages/webpack-cli/lib/bootstrap.js Show resolved Hide resolved
packages/webpack-cli/lib/webpack-cli.js Show resolved Hide resolved
@snitin315 @anshumanv
chore: suggestion
27e976c 
Co-authored-by: Anshuman Verma <anshu.av97@gmail.com>
@webpack-bot
Copy link

@snitin315 Thanks for your update.

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

@anshumanv Please review the new changes.

@alexander-akait
Copy link
Member

/cc @webpack/cli-team focus on it, it is blocker for other refactor arg parser

anshumanv
anshumanv approved these changes Nov 9, 2020
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.

Thanks

alexander-akait
alexander-akait approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Feel free to merge after CI green

@alexander-akait
Copy link
Member

random fail on macos, let's ignore, we will return to this if it happens again

@alexander-akait alexander-akait merged commit 09bd812 into master Nov 9, 2020
@alexander-akait alexander-akait deleted the fix/stats-color branch November 9, 2020 20:19
@snitin315 snitin315 changed the title fix: allow to disable stats color via --no-color fix: respect --color/--no-color arguments Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anshumanv anshumanv anshumanv approved these changes

@alexander-akait alexander-akait alexander-akait approved these changes

@rishabh3112 rishabh3112 rishabh3112 left review comments

Assignees
No one assigned
Labels
PR: reviewed-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

webpack-cli@v4.2.0 breaks color support on CI/CD
5 participants
@snitin315 @alexander-akait @anshumanv @webpack-bot @rishabh3112