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

Add Yargs default and middleware logic to display helpful message during invalid arg/command calls #262

Closed
wants to merge 7 commits into from

Conversation

mikedotexe
Copy link
Contributor

@mikedotexe mikedotexe commented Feb 3, 2020

Regarding #259
Followed this from the yargs documentation.

Added a couple of cases where an argument, command, and multiple commands are entered an not understood. Provides end user with simple feedback and instructions.

Screenshot 2020-02-03 11 52 01

Update: please see comments below regarding evolved approach

@vgrichina
Copy link
Contributor

@mikedotexe see here #259 (comment), I think we should do it differently, so that arguments are also validated.

However feel free to merge it as a quick fix for now if full fix requires much more time.

Copy link
Contributor

@janedegtiareva janedegtiareva left a comment

Choose a reason for hiding this comment

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

This works. There is also a strict mode in yargs that we may use, but we need to be sure that all possible config options are documented as flags. I am ok landing this but consider the alternative first (example which didn't land here #51)

@mikedotexe mikedotexe changed the title add Yargs default for when users type an invalid command(s)/argument WIP: add Yargs default for when users type an invalid command(s)/argument Feb 5, 2020
@mikedotexe
Copy link
Contributor Author

mikedotexe commented Feb 5, 2020

Hey folks, I have prefixed this PR with "WIP:" meaning work-in-progress. It's in a good state to start reviewing, I will make some minor changes.
So I am agreed that strict() will not work for us unless we change the config, which is possible but not ideal.
@vgrichina has the right idea of using middleware. A strange combination happens when there's middleware and a default command, both of which we need. The problem is that running near without arguments or commands no longer shows the full command list like it was when running near --help
This is a pesky issue.
I did not want to sacrifice this behavior, so I ripped out and modified some logic so that we'll still show all the commands and descriptions.
I also chained .fail() onto this with some minor error logging and suggestions.

Result:
near and near --help report the same screen (tiny difference is the word "near" appears in a different spot)
near fake » Invalid command 'fake'
near fake --fakeArg hi » Invalid command 'fake' Invalid argument 'fakeArg'
near call fake » Error: Not enough non-option arguments: got 1, need at least 2 Please use the --help flag for more information.
near login fake » Invalid command 'fake'

@mikedotexe mikedotexe changed the title WIP: add Yargs default for when users type an invalid command(s)/argument Add Yargs default and middleware logic to display helpful message during invalid arg/command calls Feb 5, 2020
@vgrichina
Copy link
Contributor

@mikedotexe the issue you linked seems to have a solution:

yargs/yargs#895 (comment)

any reason why we cannot use it?

if there is bug in yargs, I'd prefer we fix it vs reimplementing half of the logic ourselves

@mikedotexe
Copy link
Contributor Author

@mikedotexe the issue you linked seems to have a solution:

yargs/yargs#895 (comment)

any reason why we cannot use it?

if there is bug in yargs, I'd prefer we fix it vs reimplementing half of the logic ourselves

The comment you mentioned uses strict() which, as I understand will always throw for contractName and walletUrl that exists in config.js
I also don't like the idea of borrowing 54 lines of code from yargs, but having the commands there made it significantly better in my opinion. This is one of those cases where I was fighting the framework.

I think I'm now understanding Jane's comment better and will look into the approach.

@ilblackdragon
Copy link
Member

What is the status with this PR?

@mikedotexe
Copy link
Contributor Author

This approach becomes difficult with near call and near view. Closing in favor of #265

@mikedotexe mikedotexe closed this Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants