Improve error handling when scanning remotes (#84) - #85
Conversation
f08f5a9 to
f8c2c21
Compare
9999years
left a comment
9999years
left a comment
There was a problem hiding this comment.
Hey, sorry it's taken me so long to get to this, work has been hectic lately. Some stylistic changes but the underlying behavior change (surfacing errors from Gerrit::new) looks good.
| fn main() -> miette::Result<()> { | ||
| fn main() { | ||
| if let Err(error) = run() { | ||
| eprint!("{error:?}"); |
There was a problem hiding this comment.
What's this look like in practice? Is it still colored and nicely formatted? I'd also like to keep the error in the logs in some format. (This change should probably be a separate PR.)
There was a problem hiding this comment.
You're right, I'll move that to a different PR.
The change looks like this:
vs
So basically:
- alignment is fixed because "Error:" is not printed in the first line,
- no extra line at the end of the outout.
komar007
left a comment
komar007
left a comment
There was a problem hiding this comment.
No worries, I adjusted the PR according to your review.
| } | ||
| } | ||
|
|
||
| pub fn gerrit(&self, gerrit_remote_name: Option<&str>) -> miette::Result<GerritGitRemote> { |
There was a problem hiding this comment.
BTW, can we get rid of gerrit_remote_name? It is always None, and if it weren't, the logic is weird - instead we could immediately query remote_url on it and immediately fail if that fails. Currently this path has an unnecessary race condition where the remote exists while self.remotes(), but disappears before self.remote_url().
| fn main() -> miette::Result<()> { | ||
| fn main() { | ||
| if let Err(error) = run() { | ||
| eprint!("{error:?}"); |
There was a problem hiding this comment.
You're right, I'll move that to a different PR.
The change looks like this:
vs
So basically:
- alignment is fixed because "Error:" is not printed in the first line,
- no extra line at the end of the outout.
Extra changes: - failure to find remote url -> continue to next remote (instead of err), - failure in Gerrit::new (cache error) -> err immediately (the next remote is unlikely to succeed).
f8c2c21 to
ec7fb82
Compare
2 participants
Also slightly improved miette error presentation by removing the
Error:prefix generated bymain.Closes #84.