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: generation of negative flags #2555

Merged
merged 5 commits into from Mar 26, 2021
Merged

fix: generation of negative flags #2555

merged 5 commits into from Mar 26, 2021

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Mar 25, 2021

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 need
Summary
fix generation of negative flags. currently, we have negative flags for all boolean flags regardless of option.negative value.

Does this PR introduce a breaking change?
No

Other information
No

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #2555 (21cf48a) into master (d039472) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2555   +/-   ##
=======================================
  Coverage   90.98%   90.98%           
=======================================
  Files          29       29           
  Lines        1464     1464           
  Branches      419      419           
=======================================
  Hits         1332     1332           
  Misses        132      132           
Impacted Files Coverage Δ
packages/webpack-cli/lib/webpack-cli.js 92.41% <ø> (ø)

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 d039472...21cf48a. Read the comment docs.

@snitin315 snitin315 marked this pull request as ready for review March 25, 2021 12:56
@snitin315 snitin315 requested a review from a team as a code owner March 25, 2021 12:56
@snitin315
Copy link
Member Author

/cc @webpack/cli-team

@alexander-akait
Copy link
Member

@snitin315 we should better to solve it, I will finish your PR, dont' worry

@alexander-akait
Copy link
Member

alexander-akait commented Mar 25, 2021

/cc @webpack/cli-team I want to clarify something here, we have three types:

  • type: 'boolean' means --option and --no-option, any boolean for CLI always have positive and negative value
  • type: 'enum' and values: [true] means only --option (no negative)
  • type: 'enum' and values: [false] means only --no-option (no positive)

Here situation when you can use type: 'enum' and values: [false, true], it is possible, but it doesn't make sense, because it means boolean

@alexander-akait
Copy link
Member

Also need review, we should merge it before release

OPTIONS.md Outdated Show resolved Hide resolved
@snitin315
Copy link
Member Author

Here situation when you can use type: 'enum' and values: [false, true], it is possible, but it doesn't make sense, because it means boolean

Yes 'boolean' should be preferred here.

anshumanv
anshumanv previously approved these changes Mar 25, 2021
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.

Much better

@alexander-akait
Copy link
Member

@snitin315 Let's wait green #2553 and merge, then rebase

@alexander-akait
Copy link
Member

/cc @webpack/cli-team need merge it the next, I think time to release

@anshumanv anshumanv merged commit f26ebc1 into master Mar 26, 2021
@anshumanv anshumanv deleted the fix/negative-flags branch March 26, 2021 12:01
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

4 participants