Skip to content
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

Restructure how subcommands work #161

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Restructure how subcommands work #161

wants to merge 1 commit into from

Conversation

takluyver
Copy link
Member

I'm not convinced that this is actually an improvement - it looked better in my imagination - but having done it I thought I'd open a PR to consider it.

In master, all the command line parsing code is in one place, and it dispatches to individual bits of functionality in other modules. This PR puts the command line parsing for each subcommand into the module with the code it calls.

Pros:

  • Arguments are associated with the code that uses them.
  • Nicer API for defining subcommands.
  • Slightly nicer help message (I spent a while tweaking it to be just how I want).

Cons:

  • CLI definition is no longer together in one place.
  • More code to maintain.
  • Some bits use private attributes of argparse objects.

@djmoch
Copy link

djmoch commented Sep 3, 2018

I don’t know if you’re looking for comments or not, but I’ll offer some feedback anyway in the hope that it’s useful.

In short, if your goal is to separate your main entry point logic from the argument parsing logic, I think a separate cli.py file would make more sense. Mixing the argument parsing into the “business logic” just creates new confusion in place of the old.

@takluyver
Copy link
Member Author

Thanks. My thinking was to create a nice framework for defining subcommands, because I don't much like the way argparse does it. But when I actually implemented it, I didn't like the way it spread argument parsing stuff into different modules.

So this PR isn't going anywhere in its current state, but it's lingering as a kind of placeholder in case I get another idea about subcommand handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants