search

AntelopeIO / DUNES

Docker Utilities for Node Execution
Other
26 stars 17 forks source link

DUNE - invalid help for some of the options #123

Closed mikelik closed 1 year ago

mikelik commented 1 year ago

When executing dune --help we get:

--create-account ['NAME', 'CREATOR (Optional)', 'PUB_KEY (Optional)', 'PRIV_KEY (Optional)'] [['NAME', 'CREATOR (Optional)', 'PUB_KEY (Optional)', 'PRIV_KEY (Optional)'] ...]
                        create an EOSIO account and an optional creator (the default is eosio)
  --system-newaccount ['NAME', 'CREATOR (Optional)', 'PUB_KEY (Optional)', 'PRIV_KEY (Optional)', '-- FLAGS (Optional)'] [['NAME', 'CREATOR (Optional)', 'PUB_KEY (Optional)', 'PRIV_KEY (Optional)', '-- FLAGS (Optional)'] ...]
                        create an EOSIO account with initial resources using "cleos system newaccount" command. Optional flags are of the form: "-- --buy-ram-bytes 3000"
  --create-cmake-app ['PROJ_NAME', 'DIR'] ['PROJ_NAME', 'DIR']
                        create a smart contract project at from a specific host location
  --create-bare-app ['PROJ_NAME', 'DIR'] ['PROJ_NAME', 'DIR']
                        create a smart contract project at from a specific host location

(...)
  --deploy ['DIR', 'ACCOUNT'] ['DIR', 'ACCOUNT']
                        deploy a smart contract and ABI to account given
(...)
  --send-action ['ACCOUNT', 'ACTION', 'DATA', 'PERMISSION'] ['ACCOUNT', 'ACTION', 'DATA', 'PERMISSION'] ['ACCOUNT', 'ACTION', 'DATA', 'PERMISSION'] ['ACCOUNT', 'ACTION', 'DATA', 'PERMISSION']
                        send action to account with data given and permission
  --get-table ['ACCOUNT', 'SCOPE', 'TABLE'] ['ACCOUNT', 'SCOPE', 'TABLE'] ['ACCOUNT', 'SCOPE', 'TABLE']
                        get the data from the given table

which are incorrect in terms of number of arguments, because you can't run i.e. dune --deploy /home/dir1 eosio /home/dir2 eosio They all are caused by incorrect nargs usage. Probably validation has to be added for each of those options.

Possible solutions

ScottBailey commented 1 year ago

This issue exists for multiple CLI arguments.

jolly-fellow commented 1 year ago

IMHO instead of fix this exact problem it would be better to redesign the whole interface while it is not too late and there are no other software dependent on DUNE. The new interface should be hierarchical and grouped by commands and their arguments like the antler-proj. IMHO such reformatting of the interface will take comparable time with try to fix all problems which gives chaotic set of parameters as it is now. Because parsing of the command line will be much easier and number of the parameters will decrease. I have a vision of this new interface and can describe it.

I understand that this issue #123 can be solved by adding of several crutches. But DUNE actually has a problem in command line parser because the command line parameters have chaotic syntax. It is difficult to remember them and one day we will meet a problem when we will be unable to add a new parameter because of conflict with other parameters.

It make sense to redesign it, make it less ambiguous, use hierarchical approach with commands and their arguments, define a subparser for each command and describe the arguments correctly. In this case the parser will be able to generate correct help, recognize optional and demanded arguments correctly and we will not need to parse the arguments manually and add to their description "(Optional)". Parser should understand it itself. Anyway it is impossible to implement parsing of many optional arguments without a creation of a subparser object or manual parsing of the arguments. So I prefer to do it in a harmonious well designed system instead of adding crutches into the existing system.

jolly-fellow commented 1 year ago

Here is my description of DUNE's CLI parser problems and proposal how to solve them all https://github.com/AntelopeIO/DUNE/issues/129

jolly-fellow commented 1 year ago

It was agreed with @larryk85 to have the only set of the options for these commands. I will update declaration of these options and check if they work correctly.

jolly-fellow commented 1 year ago

I did a little research and found out that it is impossible to force argparse to generate a correct and beautiful help and usage descriptions for options with variable number of parameters. The developers of argparse discussing during last 10 years and still can't decide exactly how metavar should works with variable number of parameters and tuples so it works weird. It looks like the only good solution is to create a subparser for each option with variable number of parameters (nargs="*" or "+") or better add subcommands and group options under them. Or change argparse to another library. Anyway it leads to complete redesign, rewriting and retesting of the CLI parser of DUNE.

Generally IMHO rewriting and testing of the whole CLI parser is not a high priority task for now. So I propose a small fast solution which doesn't solve the problem but makes the help text more correct and understandable in my taste.

Example of an option how it looks now:

Usage text:

[--update-dep ['PROJ_DIR', 'OBJ_NAME', 'DEP_NAME', 'LOCATION (Optional)', 'TAG/RELEASE (Optional)', 'HASH (Optional)'] [['PROJ_DIR', 'OBJ_NAME', 'DEP_NAME', 'LOCATION (Optional)', 'TAG/RELEASE (Optional)', 'HASH (Optional'] ...]]

Help text:

--update-dep ['PROJ_DIR', 'OBJ_NAME', 'DEP_NAME', 'LOCATION (Optional)', 'TAG/RELEASE (Optional)', 'HASH (Optional)'] [['PROJ_DIR', 'OBJ_NAME', 'DEP_NAME', 'LOCATION (Optional)', 'TAG/RELEASE (Optional)', 'HASH (Optional)'] ...] Update a dependency in the given smart contract project

How it will looks:

Usage text:

[--update-dep ...]]

Help text:

--update-dep ...] Arguments: 'PROJ_DIR' 'OBJ_NAME' 'DEP_NAME' 'LOCATION (Optional)''TAG/RELEASE (Optional)' 'HASH (Optional)' Update a dependency in the given smart contract project

What was changed:

self._parser.add_argument('--update-dep', nargs="+", metavar='\b', help="Arguments: 'PROJ_DIR' 'OBJ_NAME' 'DEP_NAME' 'LOCATION (Optional)' 'TAG/RELEASE (Optional)' 'HASH (Optional)' Update a dependency in the given smart contract project")

As you can see it is easy to change and test. So I propose to make this small update and postpone rewriting of the whole parser to time when we will add all needed options and will have more time for refactoring of the code.

BenjaminGormanPMP commented 1 year ago

Per 3/28/23 Stand-up meeting. Escalation – @larryk85 to confirm issue #123 can be postponed.

jolly-fellow commented 1 year ago

Per 3/28/23 Stand-up meeting. Escalation – @larryk85 to confirm issue #123 can be postponed.

I don't ask to postpone it I ask to make decision what should I do:

  1. I will make a PR to the fast solution as described. It shouldn't take more than one day.
  2. I will rewrite the whole parser. I don't know how much it takes.
  3. I will postpone fixing of this bug and all which takes to the CLI interface and switch to something else if there are tasks with higher priority.