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.

Sign up for GitHub

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

Jump to bottom

Don't distinguish between command types and their handlers #1546

Closed
SuperFluffy opened this issue Sep 23, 2024 · 0 comments · Fixed by #1568
Closed

Don't distinguish between command types and their handlers #1546

SuperFluffy opened this issue Sep 23, 2024 · 0 comments · Fixed by #1568
Labels
cli pertaining to the cli

Comments

@SuperFluffy
Copy link
Member

SuperFluffy commented Sep 23, 2024 •
edited by sync-by-unito bot
Loading

Astria-cli distinguishes between the Cli container, which provides the entry point to the command line utility, and their various handlers that take the parsed CLI options as an argument.

This separation is confusing and arguably not idiomatic.

astria-cli should be refactored such that each subcommand derives the clap::Args, but also provides an entrypoint fn run(self), i.e.:

#[derive(Args, Debug)]
struct TheSubcommand {
    // options, flags, etc
}

impl TheSubcommand {
    fn run(self) -> Result<T, E> {
        // use the parsed arguments
    }
}

┆Issue Number: ENG-834
@SuperFluffy SuperFluffy mentioned this issue Sep 26, 2024
Merged
@SuperFluffy SuperFluffy added the cli pertaining to the cli label Sep 26, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 2, 2024
@SuperFluffy
refactor(cli): merge argument parsing and command execution (#1568)
29a5d19 
## Summary
Refactors the CLI to give it a cleaner structure.

## Background
The CLI was split into a `cli` module that was concerned with parsing
arguments, and a `commands` which consumed the argument data structures.

The patch clears this up by implementing inherent `<Args>::run` methods
on all leaf argument types, so that the implementation follows the same
structure for all its effects.

Note that this match does restructure the public facing command line
(except for a few lines of documentation). It merely restructures the
implementation.

## Changes
- Refactors each type deriving `clap::Args` to have an inherent method
`run`.
- Flattens the crate structure to not be unnecessarily nested.

## Testing
These will be tested in follow PRs. Smoke tests using a subset of the
astria CLI commands should still work.

## Related Issues
Closes #1546
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cli pertaining to the cli
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(cli): merge argument parsing and command execution astriaorg/astria
1 participant
@SuperFluffy