Skip to content

Restructure how subcommands work#161

Open
takluyver wants to merge 1 commit into
mainfrom
refactor-subcmds
Open

Restructure how subcommands work#161
takluyver wants to merge 1 commit into
mainfrom
refactor-subcmds

Conversation

@takluyver

Copy link
Copy Markdown
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

djmoch commented Sep 3, 2018

Copy link
Copy Markdown

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
Copy Markdown
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