-
Notifications
You must be signed in to change notification settings - Fork 40
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
find: Changed exit code when an error is encountered in process_dir()
.
#352
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
+ Coverage 59.33% 59.52% +0.18%
==========================================
Files 30 30
Lines 3819 3822 +3
Branches 869 869
==========================================
+ Hits 2266 2275 +9
+ Misses 1221 1218 -3
+ Partials 332 329 -3 ☔ View full report in Codecov by Sentry. |
Can you please add a test to ensure we don't regress in the future? |
OK, I've added a test for this fix. |
src/find/mod.rs
Outdated
@@ -149,10 +150,14 @@ fn process_dir<'a>( | |||
// WalkDirIterator::skip_current_dir for explanation. | |||
let mut it = walkdir.into_iter(); | |||
while let Some(result) = it.next() { | |||
let mut matcher_io = matchers::MatcherIO::new(deps); |
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.
What's the reason to move this here from the Ok
case?
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.
Sorry, I forgot to move it back into Ok()
.
My initial writing method was to write the error flag in matcher_io
(which means matcher_io
needs to be used in Err
). Later, I found that it was designed as an immutable structure and then passed it as a parameter to the process_dir()
function.
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.
Changes have been committed in the latest. Thank you.
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 agree with your comment that using has_some_process_error
is not a very elegant solution :)
I think you can eliminate the variable by using set_exit_code
from uucore::error
instead. It's what we use in coreutils in similar situations.
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.
Looks better! Code has been reverted and uses uucore::error
.
to be honest, I didn't know about this function before.
src/find/mod.rs
Outdated
if has_some_process_error { | ||
return Err("There are some process errors.".into()); | ||
} |
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.
GNU find
doesn't show an error after the processing, it only shows the errors you show on line 157.
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 code has been removed in the new commit. Thanks.
Good work, thanks :) |
fix: #27