-
Notifications
You must be signed in to change notification settings - Fork 63
Switch subcommand parsing to an enum Subcommand
#666
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,12 +6,10 @@ | |
| //! | ||
| //! Act as a proxy for the host serial console when it is jumpered to the SP. | ||
|
|
||
| use clap::Parser; | ||
| use humility_cli::{ExecutionContext, HumilitySubcommand}; | ||
| use std::path::PathBuf; | ||
|
|
||
| use clap::{CommandFactory, Parser}; | ||
|
|
||
| use humility_cmd::Command; | ||
|
|
||
| #[cfg(not(windows))] | ||
| mod posix; | ||
|
|
||
|
|
@@ -20,14 +18,15 @@ use posix::console_proxy; | |
|
|
||
| #[cfg(windows)] | ||
| fn console_proxy( | ||
| _args: UartConsoleArgs, | ||
| _context: &mut humility_cli::ExecutionContext, | ||
| ) -> anyhow::Result<()> { | ||
| anyhow::bail!("the console-proxy subcommand is not available on Windows") | ||
| } | ||
|
|
||
| #[derive(Parser, Debug)] | ||
| #[clap(name = "console-proxy", about = env!("CARGO_PKG_DESCRIPTION"))] | ||
| struct UartConsoleArgs { | ||
| pub struct UartConsoleArgs { | ||
| #[clap( | ||
| long, | ||
| short = 'T', | ||
|
|
@@ -108,10 +107,9 @@ enum UartConsoleCommand { | |
| Client, | ||
| } | ||
|
|
||
| pub fn init() -> Command { | ||
| Command { | ||
| app: UartConsoleArgs::command(), | ||
| name: "console-proxy", | ||
| run: console_proxy, | ||
| pub type Args = UartConsoleArgs; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth just putting the Right now, we use a sort of inconsistent mix of |
||
| impl HumilitySubcommand for UartConsoleArgs { | ||
| fn run(args: Args, context: &mut ExecutionContext) -> anyhow::Result<()> { | ||
| console_proxy(args, context) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,13 +200,12 @@ | |
| //! show only the calls to specific IPC interfaces from specific tasks. | ||
|
|
||
| use anyhow::{Result, bail}; | ||
| use clap::{CommandFactory, Parser, ValueEnum}; | ||
| use clap::{Parser, ValueEnum}; | ||
| use colored::Colorize; | ||
| use humility::core::Core; | ||
| use humility::hubris::*; | ||
| use humility::reflect::{self, Load, Value}; | ||
| use humility_cli::ExecutionContext; | ||
| use humility_cmd::Command; | ||
| use humility_cli::{ExecutionContext, HumilitySubcommand}; | ||
| use humility_doppel::{CountedRingbuf, CounterVariant, Counters}; | ||
| use indexmap::IndexMap; | ||
| use std::collections::BTreeMap; | ||
|
|
@@ -218,7 +217,7 @@ mod ipc; | |
| // This attribute means that any the args defined in `Options` will conflict | ||
| // with the `list` subcommand. | ||
| #[clap(args_conflicts_with_subcommands = true)] | ||
| struct CountersArgs { | ||
| pub struct CountersArgs { | ||
| #[clap(subcommand)] | ||
| command: Option<Subcmd>, | ||
|
|
||
|
|
@@ -323,9 +322,11 @@ enum Output { | |
| const LIST_HINT: &str = "use `humility counters list` to list all \ | ||
| available counters"; | ||
|
|
||
| fn counters(context: &mut ExecutionContext) -> Result<()> { | ||
| fn counters( | ||
| subargs: CountersArgs, | ||
| context: &mut ExecutionContext, | ||
| ) -> Result<()> { | ||
| let hubris = &context.cli.archive()?; | ||
| let subargs = CountersArgs::try_parse_from(&context.cli.cmd)?; | ||
|
|
||
| if let Some(Subcmd::Ipc(ipc)) = subargs.command { | ||
| let core = &mut *context.cli.attach_live_or_dump_match(hubris)?; | ||
|
|
@@ -622,6 +623,9 @@ fn hint() -> impl std::fmt::Display { | |
| "hint:".bold() | ||
| } | ||
|
|
||
| pub fn init() -> Command { | ||
| Command { app: CountersArgs::command(), name: "counters", run: counters } | ||
| pub type Args = CountersArgs; | ||
| impl HumilitySubcommand for Args { | ||
| fn run(args: Self, context: &mut ExecutionContext) -> Result<()> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we don't even use the alias, we use |
||
| counters(args, context) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we have this pattern of proxying the trait impl to a free function? It seems like a lot of syntactic noise for not a lot of benefit. Could this function just be the
runmethod ofHumilitySubcommand? Same comment for roughly all the other subcommands.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could, but I was trying to minimize the diff here for ease of review. We could do a follow-up PR to mechanically inline everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you! I think if you put the trait impls where the free funcs used to be, it would diff pretty cleanly, but happy to defer that since it's just a nit anyway.