Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

some cli options should actually be sub commands #71

Closed
Granitosaurus opened this issue Feb 6, 2018 · 6 comments
Closed

some cli options should actually be sub commands #71

Granitosaurus opened this issue Feb 6, 2018 · 6 comments
Assignees

Comments

@Granitosaurus
Copy link

Some options are clearly just commands and should probably be treated as such:

e.g.
nordnm sync --update should be nordnm sync update and nordnm list --countries should be nordnm list countries.

What do you think of porting cli logic to click instead of imo messy argparse? Click would also allow easy decorator solutions for issues like: #70

I'm very fond of click interfaces myself and could probably work on a PR for this :)

@chadsr
Copy link
Owner

chadsr commented Feb 6, 2018

I totally agree that most arguments should be sub-commands. I will admit that I mostly settled for the optional arguments due to how frustrating argparse can get. I have not come across click before, so I will look into porting to it. If you want to work on it yourself, that is also welcomed. The whole initiation should probably be cleaned up at the same time.

Thanks for the feedback!

@chadsr
Copy link
Owner

chadsr commented Feb 6, 2018

And with the sync --update option, I was actually thinking of removing it and adding a flag such as --no-update instead. Since the last version, nordnm already only downloads new configurations if they have changed (instead of every time), making this acceptable to be an automatic routine, which can be optionally disabled.

@chadsr
Copy link
Owner

chadsr commented Feb 11, 2018

@Granitosaurus let me know if you would like to be assigned to this issue (If you are wanting to work on a PR), otherwise I will assign myself and get working on it when I have some time.

Thanks!

@Granitosaurus
Copy link
Author

@chadsr Yup, you can assign it to me. I've already started, though I might have gone a bit to far with refactoring and such. Either way I'll submit a WIP PR and we can discuss what's good what's bad there :)

@chadsr
Copy link
Owner

chadsr commented Feb 12, 2018

That's totally fine. A lot of the current codebase is in need of major refactoring. I have been holding it off, because I still have the intentions to rehaul the whole project into a provider-agnostic package (not just for NordVPN) some time soon. Feel free to break down your work into multiple PR, so we can discuss each group of changes separately.

Thanks!

@chadsr
Copy link
Owner

chadsr commented Feb 18, 2018

Closing for now, as further discussion can be moved to PR #74.

@chadsr chadsr closed this as completed Feb 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants