-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
feat(webpack-cli): allow negative property for cli-flags #1668
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also remove the flags which are explicitly defined in cli-flags, eg no-stats
etc
Yes, but they will be no longer available in help output as well. Should I remove then ? |
Can't we filter the flags which are negative and provide a list to show up in help? |
Yes we can list all the negative flags together in help output. |
Not sure if bunching them all would be ideal, let's see /cc @webpack/cli-team need small discussion |
I think a better approach would be to not register the negated entries listed in
// Prevent default behavior for standalone options
- parserInstance.option(flagsWithType, option.description, option.defaultValue).action(() => {});
+ if (!option.name.startsWith('no-')) {
+ parserInstance.option(flagsWithType, option.description, option.defaultValue).action(() => {});
+ } IMHO removing those entries from |
@jamesgeorge007 We are still show negative flags in help, it is just sugar |
@evilebottnawi the workaround that I suggested above won't affect the help information. Instead, it makes sure that |
c3ebc11
to
00d27bb
Compare
80a7c9b
to
6941847
Compare
Both the strategies seem to be fine, I don't see any harm in having negated flags listed within Negated Flags
--no-hot negates hot
--no-stats negates stats
|
Let's list down negated flags in help output together, we can always improve help output later if needed or as per feedback. |
@jamesgeorge007 I agree with @snitin315 , we can improve it late, you can be champion 🥇 Maybe we should not list all the negative flags. We can describe the fact that all boolean flags can be negative and provide small example |
Sounds good 👍 |
dd62ecc
to
afadcd1
Compare
afadcd1
to
9e22efa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments.
687e560
to
c1893de
Compare
@snitin315 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @jamesgeorge007 Please review the new changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @snitin315 🎉, Left a small suggestion rest lgtm.
/cc @anshumanv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
What kind of change does this PR introduce?
Feature
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
NA
Summary
allow cli-flags to have a
negative
property. ifnegative: true
forflag-name
then--no-flag-name
is valid.for #1630
Resolve #1240
Does this PR introduce a breaking change?
Other information