refactor(BREAKING): remove anyhow#173
Conversation
Replace the opaque `is_async` boolean with two orthogonal flags — `is_async` (was `&`) and `ends_line` (followed by `\n` / `\r\n`) — so consumers can apply different semantics per boundary. In particular, `cmd &\nnext` is now represented as `is_async: true, ends_line: true` instead of losing the newline.
Abort a sequential list at the first non-zero command when the errexit option is enabled. Failure inside `&&` / `||` chains or pipelines is unaffected, matching bash semantics.
bartlomieju
left a comment
There was a problem hiding this comment.
Clean refactor — anyhow is fully gone, all platforms green, and narrowing the ShellPipeReader/Writer signatures to std::io::Result is a nice improvement. A couple of design-level notes inline:
- The biggest thing I'd push back on is the
monch::ParseErrorFailureErrorleak into the public signature ofparse(). You note it in the PR body, but worth deciding whether to re-export the type from the crate root or wrap it in a crate-local newtype — otherwise every downstream caller that wants to name the error type has to addmonchas a direct dep. - The
From<io::Error>(and friends) impls onShellCommandErrorstringify viato_string(), which drops the source chain. Probably fine given the type's documented "lightweight stderr reporting" use case, but flagging because it's a real behavior change vs. anyhow.
Nothing blocking from me, leaving as a comment review.
| } | ||
|
|
||
| pub fn parse(input: &str) -> Result<SequentialList> { | ||
| pub fn parse(input: &str) -> Result<SequentialList, ParseErrorFailureError> { |
There was a problem hiding this comment.
monch::ParseErrorFailureError is now part of the public signature of parse(), but monch isn't re-exported from the crate root. Downstream callers that want to name this error type (e.g. to map it into their own error enum) will need to add monch as a direct dependency, which is a bit awkward. Two options:
pub use monch::ParseErrorFailureError;at the crate root (cheap, but pins monch in the public API).- Wrap in a crate-local newtype like
pub struct ParseError(ParseErrorFailureError);and return that instead — keeps monch as an implementation detail.
The PR body already calls this out; just want to make sure it's a deliberate choice before this merges.
There was a problem hiding this comment.
Fixed by re-exporting monch's error.
|
|
||
| impl From<std::io::Error> for ShellCommandError { | ||
| fn from(err: std::io::Error) -> Self { | ||
| Self(err.to_string()) |
There was a problem hiding this comment.
These From impls stringify via to_string(), which means the source chain is lost — a caller can't, say, downcast to io::Error to inspect ErrorKind::NotFound. Given the type's doc comment ("lightweight 'print the message via stderr' reporting") that's probably intentional, but it's a real behavioral difference vs. the old anyhow path which preserved the chain.
If you want both the simple-message ergonomics and a preserved source, a thiserror enum would do it:
#[derive(Debug, thiserror::Error)]
pub enum ShellCommandError {
#[error("{0}")] Message(String),
#[error(transparent)] Io(#[from] std::io::Error),
#[error(transparent)] Utf8(#[from] FromUtf8Error),
// ...
}Not blocking — just want to make sure the trade-off is intentional.
There was a problem hiding this comment.
The current is fine because these all just get written as a string.
| && child.id().is_some() | ||
| { | ||
| panic!("Could not track process: {:#}", err); | ||
| panic!("Could not track process: {}", err); |
There was a problem hiding this comment.
Nit: these used to be {:#} (anyhow's alternate Debug, which prints the cause chain). Now plain {} (Display only). With the current types the underlying error is io::Error so nothing's lost, but if a future change wraps a richer error here, the panic message would silently drop the inner cause. Worth keeping in mind.
There was a problem hiding this comment.
Probably ok. This is only a debug assertion too.
Waiting on #171Breaking changes (AI summary):
Closes #172