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: create a cli executer #1255

Merged
merged 5 commits into from Feb 24, 2020
Merged

Conversation

rishabh3112
Copy link
Member

@rishabh3112 rishabh3112 commented Feb 23, 2020

What kind of change does this PR introduce?
feature

Did you add tests for your changes?
No

If relevant, did you update the documentation?
No

Summary
I am creating a CLI executer that will be used in case of invalid/unknown args.
Currently it looks like this
image

image
After this is merged, I would add this in appropriate places

Does this PR introduce a breaking change?
No

Other information
Improving intractability with cli

@anshumanv
Copy link
Member

I am creating a CLI executer that will be used in case of invalid/unknown args.

Interesting! What are we planning though? Throw this prompt when an invalid flag is supplied?

@rishabh3112
Copy link
Member Author

rishabh3112 commented Feb 23, 2020

@anshumanv
Yes, that is what I am planing for now. we may also assign it it's own flag if it seems useful.

@anshumanv
Copy link
Member

Yes, that what I am planing for now. we may also assign it it's own flag if it seems useful.

Sounds fair, though I think adding it just as a separate flag would be better since we for now show help on detecting any invalid flag, so user can correct any typos they made, it would be slower to show a prompt and ask them to enter the flags over again rather than correct just some invalid flag.

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@rishabh3112
Copy link
Member Author

rishabh3112 commented Feb 23, 2020

Sure it may slows down but as a CLI our first focus should be on user's successful interaction with the CLI i.e. helping user getting their work done instead of keep on correcting them.

@anshumanv
Copy link
Member

Sure it slows down but as a CLI our first focus should be on user's successful interaction with the CLI i.e. helping user getting their work done instead of keep on correcting them.

Ah didn't see the flag picker screenshot before, that can help surely, let's see how this turns out.

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Please review the approach of the algorithm. There's too much going on that doesn't help understanding what the code is doing. Also, if you believe you're doing something hacky or strange, please comment the code

packages/webpack-cli/lib/utils/cli-executer.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/utils/cli-executer.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/utils/cli-executer.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/utils/cli-executer.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/utils/cli-executer.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/utils/cli-executer.js Outdated Show resolved Hide resolved
@rishabh3112
Copy link
Member Author

Thanks for the detailed review :)

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

@rishabh3112
Copy link
Member Author

Thanks! updated the code now.

@webpack-bot
Copy link

@rishabh3112 Thanks for your update.

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

@jamesgeorge007 Please review the new changes.

@rishabh3112
Copy link
Member Author

Added descriptions as per suggestion by @ematipico

image

Copy link
Contributor

@wizardofhogwarts wizardofhogwarts left a comment

Choose a reason for hiding this comment

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

Lgtm

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.

Great work 👏

@ematipico ematipico merged commit c74574b into webpack:next Feb 24, 2020
anshumanv pushed a commit to anshumanv/webpack-cli that referenced this pull request Feb 27, 2020
* feat: create cli executer

* chore: remove unused variables

* fix: code style

* chore: use for await

* chore: add descriptions for options
@rishabh3112 rishabh3112 deleted the feat/cli-runner branch March 30, 2020 17:07
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

6 participants