Construct ExecutionContext in humility-bin#677
Conversation
mkeeter
force-pushed
the
mkeeter/context-building
branch
from
b85f4b5 to
92ebd5d
Compare
mkeeter
force-pushed
the
mkeeter/subcommand-enum
branch
from
f49aa94 to
20d913b
Compare
jamesmunns
left a comment
jamesmunns
left a comment
There was a problem hiding this comment.
A lot of pedantic nits, feel free to take them or leave them :)
| if let Ok(e) = env::var("HUMILITY_PROBE") { | ||
| cli.probe = Some(e); | ||
| } else if let Ok(e) = env::var("HUMILITY_IP") { | ||
| cli.ip = Some(e.parse().unwrap_or_else(|x| match x {})); |
There was a problem hiding this comment.
Wait how is parse returning Infallible here?
There was a problem hiding this comment.
Over in #668, we made IP parsing lazier – the parse captures a Result<..> which is only checked if we're actually using the IP address.
| // dump. If the user has specified no command line options but | ||
| // multiple of these in the environment, they will get the first in | ||
| // this ordering that we find. | ||
| if cli.dump.is_none() |
There was a problem hiding this comment.
Leaving a note, but it's probably not worth fixating on because this isn't new code:
It feels like there should be a nicer way to specify this, or at least, the semantics of what we've chosen as "the acceptable set of configuration combinations" feels overly complicated. Especially the repeated "is_some", matches, and asserts. I feel like I need a flowchart to follow this now :D
I have no calibration for "necessary complexity" vs "incidental complexity" here, but just leaving a note as an uninformed newcomer that "whew, this feels like a lot". It might be worth having a couple of functions for each acceptable source/combo of options to make it easier to hang documentation off of, so folks know what is/isn't allowed.
| // Cannot specify a dump/probe/IP address and also an | ||
| // environment and target | ||
| // | ||
| assert!(cli.dump.is_none()); |
There was a problem hiding this comment.
Maybe turn these asserts into ExitCode::FAILUREs too, to be polite?
There was a problem hiding this comment.
Mutual exclusion between these options is enforced by clap; I'll add a comment.
| PROBES_ENABLED | ||
| ); | ||
| if cmd.is_some() { | ||
| humility::warn!("ignoring subcommand after printing version"); |
There was a problem hiding this comment.
Already in scope
| humility::warn!("ignoring subcommand after printing version"); | |
| warn!("ignoring subcommand after printing version"); |
There was a problem hiding this comment.
Good catch, it wasn't in scope in the previous file that I copied from
| // The --version (-V) and --list-targets operations are implemented as | ||
| // flags, rather than subcommands, so we'll special-case them here. | ||
| if cli.version { | ||
| eprintln!( |
There was a problem hiding this comment.
When do we use humility::warn!, humility::msg!, and eprintln!?
There was a problem hiding this comment.
Right now, it's purely based on vibes! We'll want to standardize on log, but that's further down the line.
mkeeter
force-pushed
the
mkeeter/context-building
branch
2 times, most recently
from
5a3540f to
c9f203a
Compare
mkeeter
force-pushed
the
mkeeter/context-building
branch
from
c9f203a to
3a25b11
Compare
2 participants
(staged on #666)
ExecutionContext::newis a weird function: it usually builds anExecutionContext, but sometimes bails out unceremoniously withstd::process::exit. This is because it's also handling a final set of subcommands-as-cli-flags, e.g.--list-targets.In this PR, we move all of that logic into
humility-bin, which is already designed to exit with astd::process::ExitCode(instead of callingexit(..)directly).Logic should be the same, but this moves
--list-targetshandling earlier (which should fix #597).