-
Notifications
You must be signed in to change notification settings - Fork 193
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
Replace argh with argparse #435
Conversation
Another possibility would be to switch to the click library, which however uses optparse underneath instead of argparse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andshow-backups
in python 2 - it's unfortunate argparse doesn't support thealiases
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 runningbarman
on its own does with these changes - it would be nice if we could still havebarman 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.
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? |
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. |
I did a little poking at this
Argh has an undocumented As for fixing it, I looked at argh's code a bit, and it looks like it just calls whatever function you pass as And seconding the thank you for this PR! |
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. |
This should hopefully be fine after @jthreefoot-edb's recommendation but please verify if it looks ok.
Working on it per #435 (comment)
Done.
I'll check that out as well.
|
Aliases should now work for python2. |
Another thing I just noticed testing locally is that the |
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:
argh gave the possibility to also do something like |
Agreed - this might be somewhere we need to break compatibility for the sake of keeping complexity under control. Given that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few extra comments following testing locally - this is looking pretty good though! We'll give it a final review tomorrow.
Some more caveats that I've noticed when comparing the new and old help output side by side: Running 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. |
By the way I'll be squashing the commits to one when everything is reviewed and ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
0b50a2f
to
ef2ad1c
Compare
Also another thing I noticed is that the doctext shown for each command when running |
Giving this PR a final check now - we can get autocompletion working by adding |
I'll squash the commits after the final review. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last round of feedback - sorry!
@stratakis Ok I think you're all good to squash this down to one commit ready for merging. |
de40dd7
to
195eaef
Compare
Utilize the lower level argparse module for barman's cli interface as the previously used external python-argh library has been unmaintained since 2016.
195eaef
to
c6543df
Compare
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! 🎉🎉🎉 |
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? |
The next release should be out by the end of this coming week - 19th November or earlier. |
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.