georgebrock / 1pass

A command line interface for 1Password
MIT License
988 stars 54 forks source link

Use Click for the CLI #15

Closed ghickman closed 2 years ago

ghickman commented 9 years ago

This replaces the existing CLI class with a cli function and uses the great click library to deal with common CLI paradigms.

Some points to note:

This specifically scratches my itch for installing from PyPI using pipsi.

georgebrock commented 9 years ago

I need to look at click in more detail, but I have a few big concerns:

ghickman commented 9 years ago

@georgebrock – thanks for the feedback! let me try to address your concerns:

Apologies about the exit code, I've pushed a fix which deals with this, exiting with 1.

click.echo writes to stderr when using err=True which I've used where appropriate to replace writing to stderr directly.

Classes or functions is always a hard line to draw, in my experience, especially when building CLI apps. However I think a class in this situation is likely to cause more issues with maintenance/readability given the amount of functionality the code is trying to encapsulate. To give an example: I've removed 50 lines from the original 89 lines in the cli.py module, a 54% decrease. Now this isn't a hard and fast rule but generally less lines of code is both more maintainable and readable (as an aside the entire module now fits on my 13" laptop screen!). Looking specifically at the proposed changes, I could pull the functionality that was originally in _unlock_keychain_stdin and _unlock_keychain_prompt back out into functions but I'm not sure what would be gained from that.