Replace argh with argparse

[stratakis]

This is an initial attempt to replace argh with the stdlib argparse library to resolve #398

Should be compatible across all python's supported by barman (at least locally it was). The tests for the cli pass as well. However there are still some issues to resolve, like the help command showing properly the positional arguments.

Let me know what you think of that approach.

[stratakis]

Inspired by gorakhargosh/watchdog@0191c40 and https://gist.github.com/mivade/384c2c41c3a29c637cb6c603d4197f9f

[stratakis]

Another possibility would be to switch to the click library, which however uses optparse underneath instead of argparse.

[mikewallace1979]

Thank you @stratakis for this PR - the overall approach looks reasonable and is definitely heading in the right direction.

There are a couple of blocking issues:

• The barman commands which rely on barman.__config__ being available are currently failing, e.g.:

barman show-servers all
EXCEPTION: 'NoneType' object has no attribute 'server_names'
See log file for more details.
'NoneType' object has no attribute 'server_names'
See log file for more details.
Traceback (most recent call last):
  File "/Users/michael.wallace/src/EnterpriseDB/barman/barman/cli.py", line 1495, in main
    args.func(args)
  File "/Users/michael.wallace/src/EnterpriseDB/barman/barman/cli.py", line 669, in show_servers
    servers = get_server_list(args)
  File "/Users/michael.wallace/src/EnterpriseDB/barman/barman/cli.py", line 1350, in get_server_list
    available_servers = barman.__config__.server_names()
AttributeError: 'NoneType' object has no attribute 'server_names'

• We would lose the singular forms of show-servers, list-servers and show-backups in python 2 - it's unfortunate argparse doesn't support the aliases keyword argument for python 2 but we need to keep those singular forms for backward compatibility.

Also a couple of smaller issues:

• There are still a few references to argh in the comments of the pretty_args down on L1250.
• argh added a help subcommand implicitly which does the same as what running barman on its own does with these changes - it would be nice if we could still have barman help just to keep the CLI consistent with previous versions.

If we can get those two blockers resolved then we should be able to get this merged and say goodbye to argh.

[stratakis]

Thank you for taking the time to review the changes. Indeed the PR is not ready yet, however if the approach is reasonable I'll work on addressing the issues.

Would you have any opinion on the click library? It's a different approach but it might be better overall in regards to code simplicity, with the caveat of adding an extra dependency (plus the latest versions have dropped python2 compatibility).

In addition, is there a specific use case for the python2 compatibility? Will the barman project drop it at some point?

[mikewallace1979]

We have to support it as long as EDB products are supported on platforms where python 2 is the system python (e.g. Centos/RHEL 7) so while we absolutely intend to drop support for 2.7, we can't do it any time soon.

Click looks like a good fit for barman and, all things being equal, would probably be the better choice.

[jthreefoot-edb]

I did a little poking at this

The barman commands which rely on barman.config being available are currently failing

Argh has an undocumented pre_call option that we had been using to call global_config in between the parsing of the top-level commands and the subcommands, and removing this seems to be causing the above problems because now global_config isn't getting called so barman.__config__ never gets set.

As for fixing it, I looked at argh's code a bit, and it looks like it just calls whatever function you pass as pre_call in between rounds of parsing. I tried slotting that call in after the parse_args call (as global_config takes the parsed args as input), and that seemed to fix at least some of the problems. I haven't had a chance to dig deeper into it, but hopefully this might be helpful to you.

And seconding the thank you for this PR!

[stratakis]

Using aliases for python2 is non-trivial but it should be possible by utilizing something like https://gist.github.com/sampsyo/471779 which is based on the initial patchset for CPython adding aliases to argparse. I'll see if I can integrate it with the current approach.

[stratakis]

Thank you @stratakis for this PR - the overall approach looks reasonable and is definitely heading in the right direction.

There are a couple of blocking issues:

* The barman commands which rely on `barman.__config__` being available are currently failing, e.g.:

barman show-servers all
EXCEPTION: 'NoneType' object has no attribute 'server_names'
See log file for more details.
'NoneType' object has no attribute 'server_names'
See log file for more details.
Traceback (most recent call last):
  File "/Users/michael.wallace/src/EnterpriseDB/barman/barman/cli.py", line 1495, in main
    args.func(args)
  File "/Users/michael.wallace/src/EnterpriseDB/barman/barman/cli.py", line 669, in show_servers
    servers = get_server_list(args)
  File "/Users/michael.wallace/src/EnterpriseDB/barman/barman/cli.py", line 1350, in get_server_list
    available_servers = barman.__config__.server_names()
AttributeError: 'NoneType' object has no attribute 'server_names'

This should hopefully be fine after @jthreefoot-edb's recommendation but please verify if it looks ok.

* We would lose the singular forms of `show-servers`, `list-servers` and `show-backups` in python 2 - it's unfortunate argparse doesn't support the `aliases` keyword argument for python 2 but we need to keep those singular forms for backward compatibility.

Working on it per #435 (comment)

Also a couple of smaller issues:

* There are still a few references to argh in the comments of the `pretty_args` down on [L1250](https://github.com/EnterpriseDB/barman/pull/435/files#diff-d91ba4117ac85c960aa4252ff9b8df565954676cf8148c1eb07cde33f2734ee4L1250).

Done.

* argh added a `help` subcommand implicitly which does the same as what running `barman` on its own does with these changes - it would be nice if we could still have `barman help` just to keep the CLI consistent with previous versions.

I'll check that out as well.

If we can get those two blockers resolved then we should be able to get this merged and say goodbye to argh.

[stratakis]

Aliases should now work for python2.

[mikewallace1979]

Another thing I just noticed testing locally is that the diagnose command is gone - it didn't have any argh decorators but it's the cli.diagnose function. It doesn't take any arguments so probably just needs a @command() decorator.

[stratakis]

A help command was added. Since I've found no easy way unfortunately to add an alias to --help I've simply created a new subparser which prints out the help. However this does not work the same as argh:

$ barman help will print out the help but only if we are under the barman user, since the global_config is called

argh gave the possibility to also do something like $ barman help list-servers which would be equivalent to $ barman list-servers --help. That is not the case with the new subparser adding the help command. Possibly I'll have to delve again into the private attributes or argparse, however the complexity seems quite big if the argh compatibility is to be maintained 100%.

[mikewallace1979]

Possibly I'll have to delve again into the private attributes or argparse, however the complexity seems quite big if the argh compatibility is to be maintained 100%.

Agreed - this might be somewhere we need to break compatibility for the sake of keeping complexity under control. Given that the help command is nearly always going to be used interactively (I can't think of any use cases for running help in scripts) it doesn't seem too bad to change behaviour here - and the --help option is always there to provide subcommand-specific help when needed.

[mikewallace1979]

A few extra comments following testing locally - this is looking pretty good though! We'll give it a final review tomorrow.

[stratakis]

Some more caveats that I've noticed when comparing the new and old help output side by side:

Running --help now will show the dict of the positional arguments including the aliases and in a non-alphabetical order. argh would sort the arguments and would not show the aliases.

Also argh would show a list of all the subparsers as positional arguments on the help text along with their help. In the case of argparse this is for explicitly set arguments on the main parser, but not for the subparsers.

And I haven't looked yet on the shell completion with arghcomplete if things work there.

[stratakis]

By the way I'll be squashing the commits to one when everything is reviewed and ready.

[didiermichel]

Hi @stratakis,
I had a look at the code. I like the decorator.

You should try run tox -e black to format your code before merging.

[mikewallace1979]

Something like this would fix ordering of the subcommands in both the positional arguments and the expanded help, though it is yet more coupling to argparse internals: https://gist.github.com/mikewallace1979/8d9ae2fa84ce5f4fc0b70999fbd5b04c

[stratakis]

Something like this would fix ordering of the subcommands in both the positional arguments and the expanded help, though it is yet more coupling to argparse internals: https://gist.github.com/mikewallace1979/8d9ae2fa84ce5f4fc0b70999fbd5b04c

This is quite elegant, the solution I was trying was far more complex than that. Added it.

[stratakis]

Also another thing I noticed is that the doctext shown for each command when running $ barman --help is not properly wrapped on the terminal as it happens with argh, or argparse by default for that matter. A newline is added after ~50 chars instead of extending the line up to where the terminal is.

[mikewallace1979]

Giving this PR a final check now - we can get autocompletion working by adding argcomplete.autocomplete(p) just before the call to p.parse_args on L1786.

[stratakis]

I'll squash the commits after the final review.

[mikewallace1979]

@stratakis Thanks for making all the changes. I'm going to push a copy of this branch directly to the EnterpriseDB repo so that the full CI pipeline can run (there are some behind-the-scenes integration which will take around 20 minutes) and if that all succeeds then I think we're good to squash and merge.

[mikewallace1979]

One last round of feedback - sorry!

[mikewallace1979]

@stratakis Ok I think you're all good to squash this down to one commit ready for merging.

[stratakis]

Squashed and rebased on top of master. Edited the commit message as well. Should I also add anything else, such as a changelog entry?

[mikewallace1979]

Squashed and rebased on top of master. Edited the commit message as well. Should I also add anything else, such as a changelog entry?

I think we've got everything we need for this PR. We'll update the NEWS at release time and we retired the ChangeLog since it was just duplicating the commit history.

Thanks again for taking this on and sticking with it (and indeed identifying the issue in the first place) - it's great that we've been able to wave goodbye to unmaintained argh so quickly! 🎉🎉🎉

[stratakis]

Thanks as well for reviewing the PR, addressing all the issues with it and merging it in such a swift manner :)

Is there an ETA for the next release?

[mikewallace1979]

The next release should be out by the end of this coming week - 19th November or earlier.