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: help and version functionality #1972

Merged
merged 7 commits into from
Oct 21, 2020
Merged

fix: help and version functionality #1972

merged 7 commits into from
Oct 21, 2020

Conversation

snitin315
Copy link
Member

What kind of change does this PR introduce?
bugfix
Fixes #1917

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

Screenshot at 2020-10-20 15-59-27

Does this PR introduce a breaking change?
No

Other information
No

@snitin315 snitin315 requested a review from a team as a code owner October 20, 2020 10:38
@snitin315 snitin315 changed the title fix: invoke runHelp function if --help is passed fix: always output help information Oct 20, 2020
// Use customized help output if available
parser.on('option:help', () => {
// Use customized help output
if (args.includes('--help')) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. It should be removed from there.

Copy link
Member Author

Choose a reason for hiding this comment

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

The same should be done for version too.

Screenshot at 2020-10-20 16-20-20

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update 👍🏻

@snitin315 snitin315 changed the title fix: always output help information fix: help and version functionality Oct 20, 2020
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

@@ -4,11 +4,6 @@ const { run } = require('../utils/test-utils');
const helpHeader = 'The build tool for modern web applications';

describe('commands help', () => {
it('throws error if supplied as an argument for subcommands', () => {
const { stderr } = run(__dirname, ['serve', 'help'], false);
Copy link
Member

Choose a reason for hiding this comment

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

Supplying help as an argument to any of the subcommands shouldn't be allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can allow it for consistency.

webpack --flag help - Allowed
webpack help --help - Allowed
webpack help cmd - Allowed

Copy link
Member

Choose a reason for hiding this comment

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

webpack help --help interesting, but I think it is good

Copy link
Member Author

@snitin315 snitin315 Oct 20, 2020

Choose a reason for hiding this comment

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

oh!, I wanted to type webpack help --flag.
Just checked webpack help --help is allowed as well and is equivalent to webpack help

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@snitin315 Can we refactor this test to allow using help here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated 👍🏻

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 need review

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.

Inconsistent behavior of webpack --flag --help
4 participants