Improving --help: docstring format for arguments (options and flags)

[d5000]

As I wrote in issue #6, I have read through some documentation to improve how options and flags are shown in the help for each command (e.g. when you type config set --help).

First, to clarify, I guess you know that in Python there are positional arguments and keyword arguments. Positional arguments are mostly mandatory and of the form COMMAND ARG1 ARG2 ARG3, while keyword arguments can be COMMAND --argname ARG1 --argname2 ARG2 or COMMAND -a ARG_A -b ARG_B -c ARG_C.

The basic "problem" is:

Fire (the platform pacli runs on) generates automatically a section "FLAGS" in the help. This section contains all keyword arguments, i.e. options and flags which are not mandatory.

I have ignored this section until yesterday, and added the flags/options without a proper format to the docstring, so they were displayed in the DESCRIPTION section of when a command is called with --help. But that made the FLAGS section look quite "technical", as there was no description there.

But the FLAGS section can be manipulated using a certain format in the docstring. The description of each flag would then be shown in the FLAGS section, not in the DESCRIPTION section.

I have done this for the command config set in the last commit, so you can see how this would look.

First question I wanted to ask: do you think this to be an improvement?

---

Sometimes, however, arguments are mandatory positional arguments for some variants of the command only.

I would like them to be entered in the format COMMAND ARG1 ARG2 ARG3 but Fire treats them as keyword arguments.

These options are thus shown in the FLAGS section and can confuse users.

We have discussed the case of --blockheight/-b in the checkpoint set command, where you proposed to clarify in the description that the -b is optional. But I don't consider that really ideal.

I have unfortunately still not found a way to suppress an option which is meant to be used as a positional argument to be shown there. But the description of the flag can be edited, and this could improve the usability a bit.

Another option I have is to use *args instead for all positional arguments. This would require a bit of rewrite, but would be cleaner.

About this second issue I have to think still a bit, but of course you can give feedback.

[buhtignew]

But the FLAGS section can be manipulated using a certain format in the docstring. The description of each flag would then be shown in the FLAGS section, not in the DESCRIPTION section. I have done this for the command config set in the last commit, so you can see how this would look. First question I wanted to ask: do you think this to be an improvement?

It's more clean, IMO. So I think it's an improvement.

_

Another option I have is to use *args instead for all positional arguments. This would require a bit of rewrite, but would be cleaner. About this second issue I have to think still a bit, but of course you can give feedback.

The good thing about flags is that they can be used in abbreviated form. If we transform them into positional arguments it would increase the quantity of writing for the advanced users.

[buhtignew]

We have discussed the case of --blockheight/-b in the checkpoint set command, where you proposed to clarify in the description that the -b is optional. But I don't consider that really ideal.

I'm not sure to have understood why is it that that you are reluctant to consider specifying the flags position and to make it clear the flag is optional. I think it's the only way for us for the moment, at least until we don't find the way to suppress those flags in the Flags section of the help. I think that if we don't do it that way, the advanced users will find it difficult to understand how our commands work, thus they'll think our code is buggy and may just decide to abandon the idea using it. Considering how much time we are dedicating to release a neat code it would be really a pity.

[d5000]

I've more or less already decided to go forward with the *args solution I mentioned above. This would mean a clean distinction between keyword and positional arguments. Only that I still didn't implement it (bugfixing for now has higher priority). It will take me a bit of time but it is worth it just for the reasons you wrote -- this will clear all ambiguities about the flags/option keywords being optional or not.

Until then, you can simply see which flag/option keywords are optional in the DESCRIPTION part of the help.

I'll report the progress here.

The good thing about flags is that they can be used in abbreviated form. If we transform them into positional arguments it would increase the quantity of writing for the advanced users.

I don't understand what you mean here. command ARGX ARGY is still shorter than command -x ARGX -y ARGY. Positional arguments are better for all arguments which are mandatory or at least are used very frequently.

[buhtignew]

The good thing about flags is that they can be used in abbreviated form. If we transform them into positional arguments it would increase the quantity of writing for the advanced users.

I don't understand what you mean here. command ARGX ARGY is still shorter than command -x ARGX -y ARGY. Positional arguments are better for all arguments which are mandatory or at least are used very frequently.

I think the issue here is that it was not that clear for me what are the positional arguments. Now it's more clear.

Only that I still didn't implement it (bugfixing for now has higher priority). It will take me a bit of time but it is worth it just for the reasons you wrote -- this will clear all ambiguities about the flags/option keywords being optional or not. Until then, you can simply see which flag/option keywords are optional in the DESCRIPTION part of the help.

I think it would be important to implement it at this stage, because it possible that we'll need to re-test all the commands after that. Since the bugs may occur during the new setup implementation.

[d5000]

Ok it will be the next thing on my TODO list after fixing the current bugs (those related to transaction list). There are some commands affected by this, but by far not all (I think less than 20%). I'll list them here later so you can omit them while testing. When this is ready, I'll also go on with changing the docstring format so the flags/options are explained in the FLAGS section.

[buhtignew]

Maybe you can change just one command so I'll be able to provide you my feedback without you putting too much effort into it.

[d5000]

I have tried out the *args format but I have found out that it has also a problem. While it solves the ambiguity, the SYNOPSIS part of the help is then rendered in a weird way, with the flags (keyword arguments) shown before the positional arguments ...

I think we have to accept that Fire isn't really perfect when it comes to usability, be it the weird problems when it renders the help string, or these ambiguities with positional/keyword arguments.

I still hope once the tokens are launched an alternative CLI interface or even a GUI can be implemented, maybe with risen funds, but I really don't have the free time to do such a big change now myself, so we'll have to stick with Fire. I'd like at least the PoB token and PeerAssets in general to be launched at most in Q3 on mainnet because the "technical" part of the tokens is more or less ready (only some unit tests are missing).

I'll now follow your suggestion and add a small message in the help to all the affected options (those which can be positional and keyword arguments at the same time), indicating that the user can -- and should -- use those arguments in a positional way and in this case should consult the Usage modes in the DESCRIPTION part of the help. I still think this isn't ideal but Fire doesn't allow anything better. And after all there will be an updated manual which should be the standard "way to learn about the token system" for the non-technical users.

I have now changed the config set, config show and config list commands in this fashion (in reality only config set had an ambiguity here, but on the others I've changed the way Flags are displayed), so if you want you can give feedback, but I think then we can close here. I'll progressively change the other commands as well, but today I'd not so much time.

[buhtignew]

so we'll have to stick with Fire.

Agreed. _

I'd like at least the PoB token and PeerAssets in general to be launched at most in Q3 on mainnet

That's would be great! _

I have now changed the config set

I think we should change the following description for pacli config set LABEL VALUE -e/--extended CATEGORY

    Adds a setting in the basic configuration file (by default: pacli.conf)
    or a category CATEGORY in the extended configuration file.

into something like:

 Adds LABEL/VALUE pair into a CATEGORY of extended configuration file.

if I've got it right it's not possible to use it like that for the the basic file.

I also think that it's possible that pacli config set NEW_LABEL OLD_LABEL -m [-e/--extended CATEGORY] should be changed into pacli config set NEW_LABEL OLD_LABEL -m -e/--extended CATEGORY: because if it's not usable for the basic file, the -e flag and CATEGORY are becoming mandatory.

The descriptions of the flags are all great. Maybe we can reformulate the description of the -v flag from

To be used as a positional argument (without flag keyword), see 'Usage modes' above.

into something like:

Can be used before VALUE (see 'Usage modes' above) but is not mandatory.

_

config show

Maybe we can reformulate the description in help for pacli config show LABEL like this: Instead of

  Shows setting in the basic configuration file.

we can put something like:

Outputs the VALUE of the LABEL mentioned in basic configuration file

In the same way we can reformulate the description for pacli config show LABEL -e/--extended CATEGORY [options] Instead of

Shows setting in a category CATEGORY of the extended configuration file.

we can put something like

Outputs the VALUE set for the LABEL mentioned in the extended configuration file CATEGORY.

I'd also like to ask you whether it would make sense to omit the [options] in the line pacli config show LABEL -e/--extended CATEGORY [options] mentioned in the help because I don't know which options can be put there.

[d5000]

I have implemented most of your suggestions, sometimes with a different wording to use correct terminology. Some of the commands had also still outdated parts of the docstrings, which I replaced.

I'm currently redesigning the options/flags of transaction list with the goal to have one-letter "short flags" for the most common options like in the other basic commands. This was quite challenging but I'm almost there. Some of the changes will maybe affect other commands too. As I did not finish this today, there's no new update still.

[buhtignew]

Great! I'll be waiting for your update.

[d5000]

I have now found short labels for almost all commands and thus updated the code.

The only command where the flags have significantly changed is transaction list. The changes are:

    --start - became -f or --from_height
    --end - became -e or --end_height
    --burns - became -b or --burntxes
    --reftxes - became -g or --gatewaytxes
    --claims - became -c or --claimtxes
    --txids - became -i or --ids
    --all -  became -x or --xplore ("block explorer mode")
    --count - became -t or --total
    --coinbase = -v or --view_coinbase
    --raw = -l or --lraw ("listtransactions raw mode")

I would not like to change these again because it was very difficult to find flag keywords beginning with different letters for this command - only in a case it was grossly misleading, but I don't hope so :)

Other changes:

address set: --new became -f or --fresh (due to conflict with --now)
deck show: --p2th became -s or --show_p2th
deck init: --store_label became -l or --label
transaction show: --anatomy became -s or --structure (like originally, as conflict with -s was solved)
transaction show: --txid became -i or --id (reason: so it's consistent with the transaction list -i option)

The commands work exactly the same as before, only that the flag keywords are changed.

As you see, I've decided to use the word "gateway transactions" like you suggested in another issue for transactions which result in rewards of AT tokens (originaly "reftxes"). This will of course be explained well in the manual.

Additionally as I wrote I improved the wording based on your suggestions above, and removed unnecessary content. I've also decided to put only the short flag (when available) in the DESCRIPTION part, the long form should be consulted in the FLAGS section - this seems to be established practice, e.g. in the help of git.

[d5000]

Just a little update: have updated the flags from the checkpoint, token., attoken and pobtoken commands as of commit 75d2033.

Only missing ones are now podtoken, proposal and donation groups which have still docstings in old formats. Also I may change flag names from these commands still, so it's best if they're still not tested.

[d5000]

Commit cb9f8b4 extends the new format to all commands with exception of dex. Most commands have short flags for all arguments now, but a couple in the donation and proposal classes could probably see some modificiations still.

[d5000]

Please do NOT upgrade to cb9f8b4. It contains a bug which will result in errors in other commands. Sorry :)

Next update will fix this, but I'm still testing it as it contains major updates.

Until then please downgrade to 75d2033 if you have already upgraded (git checkout 75d2033)

[buhtignew]

Hope to have done it right. I've run git checkout 75d2033 and then pip install --upgrade . --break-system-packages.

[d5000]

Commit 98b11ba fixes the bug. It contains also a big upgrade for transaction list -x with the "block locating" functionality but it is not completely ready for public testing as I want to still add a couple of features. I'll open an issue once this is ready.

back on topic: dex docstrings are now also upgraded to the Google format. So this issue can be closed if there are no more questions on this topic.

[buhtignew]

So I shouldn't be testing transaction list -x for the moment while all the other commands are fine now?

[d5000]

Exactly. The -x feature should be tested once I open a specific issue for it, which perhaps I'll do until tomorrow.

I think I'm closing here then.