Add the bases for making prompt commands plugin based

[LysanderGG]

What current issue(s) does this address, or what feature is it adding?
#623

How did you solve this problem?
This PR adds base classes for prompt commands and subcommands.
It also adds a few implementations to be sure the system is working:

• create wallet
• wallet
• wallet migrate
• wallet create_addr

This PR/branch will be quite big, so it is intended to be kept the simplest possible so the behaviour of the prompt and commands should not be changed in this PR/branch.

How did you make sure your solution works?
The prompt commands should all work as before.

Are there any special changes in the code that we should be aware of?
Not particularly.

Please check the following, if applicable:

• Did you add any tests?
• Did you run make lint?
• Did you run make test?
• Are you making a PR to a feature branch or development rather than master?
• Did you add an entry to CHANGELOG.rst? (if not, please do)

What is left to do?
This PR is intended to put the bases for the refactoring.
Once this PR is merged, the following things are left to do.

• Migrate all the commands to the command classes
• Fix existing tests
• Test the newly added classes

What can be done after?

• This PR manually registers the commands, we should use an auto-discovery mechanism.
• Help messages could be improved. It would be possible to have several levels, such as command listing with help, subcommands listing with command help and detailed help with command subcommand help (help could be the first word if we want).
• Auto completion could be improved to suggest only subcommands after a command has been entered. For instance when the user has entered import, we should not suggest create_addr.

What I would like to be reviewed:

• If the system I thought of is good enough
• If the base classes are good enough

If possible, once the base is validated, I'd prefer to make several small PRs with a few classes at a time so it's easier and quicker to review and merge.

I am open to comments and suggestions!

[coveralls]

Coverage decreased (-0.6%) to 83.26% when pulling 56e48a0 on LysanderGG:refactor-prompt into ec96ae7 on CityOfZion:refactor-prompt.

[localhuman]

Looks great!

[jseagrave21]

@LysanderGG If this is implemented, I would like to help migrate the commands and tests. Perhaps we can cooperate to accomplish the goal?

[LysanderGG]

@jseagrave21 Yes we should be able to split the work once this PR is merged

[ixje]

I've played around a bit with the code seeing if I could implement the help we discussed on discord. I've got a WIP here that results in the following (don't mind the colours)

So the initial idea of registering a subcommand help within the base command doesn't work because then we cannot get access to all available __sub_commands. That prevents us from displaying <base command> help.

I thought perhaps inheriting from a base command (i.e. CommandWallet) would work i.e.
def CommandWalletHelp(CommandWallet):
Unfortunately that gave me issues where most SubCommands have class methods, but one of them would require the self argument when called (i.e. CommandWalletHelp).

I ended up with the preliminary approach in the link but it feels messy as that would put all markup/alignment responsibility with the class inheriting from CommandBase and/or SubCommandBase. Perhaps I'm just too tired at this point to see a better approach, but we at least need to expose CommandBase.__sub_commands somehow to be able to provide proper help, and then I hope we can find a more solid solution to achieve the above than what I've created.

[LysanderGG]

Thank you for having tried this!

Here are a few ideas:

1. we could define a handle_help() method in CommandBase that returns True or False based on if it processed or not. We could then call super().handle_help(arguments) in the Commands implementations' execute().
   If I am not misreading there is nothing related to the wallet in your implementation so a default implementation in CommandBase would avoid duplicated code in all Command implementations and we wouldn't need to expose __sub_commands.
   Anyway, exposing __sub_commands is not a problem imo

2. If we remove the @classmethod from SubCommandBase it could be implemented as a SubCommand but we couldn't handle the case of SubCommand's help (e.g. wallet migrate help).
   We would still need something like 1. or your implementation to handle this.
   Note that it would also need access to the CommandBase and __sub_commands but it's fine I think.
   It is sensible to have access to the BaseCommand from the SubCommands.

I thought perhaps inheriting from a base command (i.e. CommandWallet) would work i.e.
def CommandWalletHelp(CommandWallet):
Unfortunately that gave me issues where most SubCommands have class methods, but one of them would require the self argument when called (i.e. CommandWalletHelp).

I think I didn't understand this part.

3. We could check if the last argument is help in prompt.py and then call a helper function in the BaseCommand similar to 1.

[ixje]

I followed up on 1 and 2 (result) if I understood you correctly. Works well so far as I've tested it.

In summary I did the following things

• move help logic to CommandBase
• let SubCommandBase inherit from CommandBase in case they need something from it
• Make CommandDesc arguments mandatory so that we can rely on values being present when formatting instead of filtering. I think all text should be there to have a proper user experience.

Now having done this I think we might want to refactor CommandDesc.help to a list of (parameter,parameter description)'s such that we can dynamically build the help text instead of expecting the writer to \nits way to a correct help message format. See the docstrings I added for CommandDesc to see how messy that gets. Ideas?

[LysanderGG]

Thank you for this!

The points 1 and 2 were different ideas to solve the same problem, maybe it wasn't clear in my previous message, sorry.

let SubCommandBase inherit from CommandBase in case they need something from it

This is not what I meant, I was suggesting to have SubCommandBase's __init__ method take a CommandBase to be able to access it through self.__parent_command or something like this.
The goal was to be able to call the parent's command handle_help or other method.

But with the handle_help you implemented it should not be necessary because help is not a SubCommand.
I would suggest not to inherit CommandBase for now.

Now having done this I think we might want to refactor CommandDesc.help to a list of (parameter,parameter description)

Absolutely!

However it should also have a free text variable imo, I would suggest __init__(self, command, short_help, arguments = None, help = None) because:

• It is possible to have commands without arguments
• the help could be more detailed than short_help and have examples of calls or warnings or whatever.

Last thing, about the point 3:

Now that I re-read my previous message, I can see it was confusing.
I think we should move the detection of help as argument[-1] in prompt.py to avoid the following duplicated code in every execute implementation.

is_handled = super().handle_help(arguments)
if is_handled:
    return

I think it could be handled as follows in prompt.py:

• if the command is only help do as before.
• if the last argument is help call the self.commands[command].handle_help(arguments) (see https://github.com/ixje/neo-python/commit/88502dd48539668bcd03f103273c94481028fcc9#diff-50315a7978a702a9e31eda464ba277bbR986 -- we would have to handle help before checking if command in self.commands:)

We wouldn't need to return a boolean any more because we would be sure that the last argument was help.

I hope this message is less confusing 😅
What do you think?

[ixje]

Pushed it a bit more forward again. After I took a short walk and thought why not add a ParamDesc class and then we can loop over that if there are params. See the result: here

Yeah I wasn't too happy yet with the if is_handled: construct. Perhaps you can take it a step forward and move that logic prompt.py, you seem to have a good idea for it :)

Finally, I'm not sure yet where and how to call upon the long help. I don't think we'd want a long help message every time. Meaning; wallet migrate help tells you:

neo> wallet migrate help                                                                                                                                                                        

Migrate a wallet from y to z  # <- short_help

Usage: migrate {option1} {option2} (option3) # <- command + params 

option1         - description of params 1 # <- param +  param_description
option2         - description of params 2
option3         - (Optional) description of params 3

If that isn't clear enough, then perhaps we might want to call upon help which shows examples? Or we add examples to the above after the param descriptions, but I'm not sure if that's going to be too elaborate? I'm open to opinions here.

[LysanderGG]

ParamDesc is probably the cleanest way to handle it.
This looks quite good like this, maybe we don't need the help.
I personally like to have examples for complex commands in help messages but maybe it's too much indeed.

I would suggest to do without help for now and add it after if needed. Anyway it would be None by default so it won't be hard do add afterwards.

I think we've reached a good enough state, thank you for your help!

How shall we proceed now?
Should I merge your code to my branch in order to update this PR and then move the if is_handled logic to prompt.py?

[ixje]

Should I merge your code to my branch in order to update this PR and then move the if is_handled logic to prompt.py?

Sounds good!

[LysanderGG]

e592858 moves the detection of help to prompt.py.
Do we have something else to handle in this PR?

[ixje]

A couple of last touches such that we have a clean start where others can help migrate. Thanks for picking this issue up, this is going to be a good first step in cleaning up and improving usability/user experience 👍

[ixje]

I just took a moment to think about how to group all available commands and have a suggestion posted here: #623 (comment)

One thing I noticed is that we might want a 3rd level subcommand 🤔 If you look at the above proposal we might have wanted the following

neo> wallet token help
- send
- send_from
- approve
- history

neo> wallet token send help // <- this level is not possible atm

Usage: send {token symbol} {address_from} {address to} {amount}

token_symbol - description
address_from - description
address_to - description
amount - description

instead we're now forced to have

neo> wallet help
tkn_send - send tokens
tkn_send_from - send tokens from address
etc.

neo> wallet tkn_send help
Usage: tkn_send {token symbol} {address_from} {address to} {amount}

token_symbol - description
address_from - description
address_to - description
amount - description

similarly for wallet import and wallet export

[LysanderGG]

I fixed the points you raised and moved create into wallet.

About the 3rd level of subcommands, I went ahead and remove SubCommandBase.
subcommands are now inheriting CommandBase and we now support any levels of subcommands.

I made the necessssary functions recursive and added a __parent_command member to CommandBase.

Here is for the sake of and example the console output when I put create wallet as a subcommand of wallet. (It is just for the example, I changed it afterwards).

Please let me know what you think :)

[ixje]

💯
Thanks allot, this is exciting!

[LysanderGG]

Thank you for your help and reviews!
Let's move them all now 👍