-
-
Notifications
You must be signed in to change notification settings - Fork 824
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
fd --exec should fail if one of --exec fails #526 #531
Conversation
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.
This looks great. Thank you very much. Is there any way we could write an integration test for this?
src/exit_codes.rs
Outdated
|
||
impl ExitCode { | ||
pub fn error_if_any_error(results: Vec<Self>) -> Self { | ||
if results.iter().any(|s| match s { |
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.
Maybe this match
should be extracted into another function: ExitCode::is_error
?
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.
This way, we could simply run: if results.iter().any(ExitCode::is_error) { … }
src/exit_codes.rs
Outdated
} | ||
} | ||
} | ||
|
||
impl ExitCode { | ||
pub fn error_if_any_error(results: Vec<Self>) -> Self { |
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 feels a little strange to me to have this in an impl ExitCode
block. I think it could easily be a free function.
As for the naming, maybe: merge_exitcodes
?
Finally: can we please add a few unit tests for this function (in this file)?
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 think I've added a few interesting unit tests of merge_exitcodes
.
Let us know if they're good enough 😄
Thank you! |
Hi 😄 I've checked all your comments and suggestions and I'll follow them as soon as possible. I hope to be able to polish this PR before the next monthly meeting of the Open Source Saturday but unfortunately I can't promise that (I wish I could 😞) |
Hey, no worries. There's absolutely no time pressure here. We'll merge it whenever it's ready. Thank you! |
♾ 🙇 |
ef58741
to
0ef6ecc
Compare
I think I've addressed pretty much everything that was pointed out last month 🤔 I remain obviously at your total disposal for every new comments 😄 |
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.
Thank you very much for the updates! Looks great.
@sharkdp thank you once again for all your invaluable work & support! 🙇♂️ We really hope to be able to further contribute to your projects! 😄 Cheers! |
Looking forward to it! |
Me and @Giuffre tried to tackle this issue during our monthly OSS meeting 😄:
We apologize in advance if we didn't properly addressed what stated in the over mentioned issue and we remain at disposal to further work on it during our next meeting 🙇
P.S: thank you a lot for this wonderful crate! 🙇♂️