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

feat(webpack-cli): allow negative property for cli-flags #1668

Merged
merged 4 commits into from
Jul 7, 2020

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Jul 4, 2020

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. if negative: true for flag-name then --no-flag-name is valid.
for #1630

Resolve #1240

Does this PR introduce a breaking change?

Other information

@snitin315 snitin315 requested a review from a team as a code owner July 4, 2020 09:01
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.

We can also remove the flags which are explicitly defined in cli-flags, eg no-stats etc

@snitin315
Copy link
Member Author

Yes, but they will be no longer available in help output as well. Should I remove then ?

@anshumanv
Copy link
Member

Can't we filter the flags which are negative and provide a list to show up in help?

@snitin315
Copy link
Member Author

Yes we can list all the negative flags together in help output.

@anshumanv
Copy link
Member

Not sure if bunching them all would be ideal, let's see

/cc @webpack/cli-team need small discussion

@jamesgeorge007
Copy link
Member

jamesgeorge007 commented Jul 4, 2020

I think a better approach would be to not register the negated entries listed in cli-flags

parserInstance.option(flagsWithType, option.description, option.defaultValue).action(() => {});

                 // 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 cli-flags and writing custom logic to show up negated flags as part of the help information would be too much of a work as compared to this tweak.

@alexander-akait
Copy link
Member

@jamesgeorge007 We are still show negative flags in help, it is just sugar

@jamesgeorge007
Copy link
Member

@evilebottnawi the workaround that I suggested above won't affect the help information. Instead, it makes sure that negated flags aren't registered for the second time.

@jamesgeorge007
Copy link
Member

jamesgeorge007 commented Jul 5, 2020

Both the strategies seem to be fine, I don't see any harm in having negated flags listed within cli-flags considering the fact that they are standalone. But anyway if we're gonna remove, it would be better to have them in a separate section as part of the help information:-

Negated Flags

  --no-hot      negates hot
  --no-stats    negates stats

@snitin315
Copy link
Member Author

Let's list down negated flags in help output together, we can always improve help output later if needed or as per feedback.

@alexander-akait
Copy link
Member

@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

@jamesgeorge007
Copy link
Member

@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 👍

@snitin315
Copy link
Member Author

Screenshot at 2020-07-06 20-36-16

Copy link
Member

@jamesgeorge007 jamesgeorge007 left a 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.

packages/webpack-cli/lib/utils/arg-parser.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/groups/HelpGroup.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/utils/arg-parser.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/groups/HelpGroup.js Outdated Show resolved Hide resolved
@webpack-bot
Copy link

@snitin315 Thanks for your update.

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

@jamesgeorge007 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.

Great work @snitin315 🎉, Left a small suggestion rest lgtm.

packages/webpack-cli/lib/groups/HelpGroup.js Outdated Show resolved Hide resolved
@alexander-akait
Copy link
Member

/cc @anshumanv

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.

💯

@snitin315 snitin315 merged commit e7b46c2 into next Jul 7, 2020
@snitin315 snitin315 deleted the feat/negative-flags branch July 7, 2020 11:19
@snitin315 snitin315 added this to Done (Phase 2) in GSoC 2021 Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
GSoC 2021
  
Done (Phase 2)
Development

Successfully merging this pull request may close these issues.

Negated flags
6 participants