-
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: Implement -newerXY
#342
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #342 +/- ##
==========================================
+ Coverage 58.59% 59.12% +0.53%
==========================================
Files 30 30
Lines 3712 3736 +24
Branches 840 847 +7
==========================================
+ Hits 2175 2209 +34
+ Misses 1223 1218 -5
+ Partials 314 309 -5 ☔ View full report in Codecov by Sentry. |
Could you please add unit and integration tests? Thanks |
Okay, I will add the test code as soon as possible. |
Thanks |
@hanbings apart from the tests, is the code ready for review? |
I'm sorry I just saw the message. But yeah, ready to review. Thank you. |
src/find/matchers/time.rs
Outdated
// accessed time test | ||
let accessed_matcher = NewerTimeMatcher::new(NewerOptionType::Accessed, time); | ||
|
||
thread::sleep(Duration::from_secs(2)); |
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.
why sleep ?
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 is my oversight, sleep() is not really needed here, and there are two other codes with the same problem. They have been modified in the commit. Thanks!
src/find/matchers/time.rs
Outdated
|
||
let created_matcher = NewerTimeMatcher::new(NewerOptionType::Birthed, time); | ||
|
||
thread::sleep(Duration::from_secs(2)); |
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.
same, is this sleep needed?
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 sleep() is used to ensure that there is a small distance between the time when assert() is executed and the time when the file was created. In fact, this code fails the test on my machine after removing the sleep().
and...
Currently the i64 type millisecond timestamp is used to determine the time. I have noticed that it is not accurate enough and I am going to change it to microsecond.
because the time parameter input does not support microsecond or even millisecond input, millisecond seem to be sufficient.
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.
two seconds is long, try 0.1 ?
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.
OK, committed. Thanks!
link: #307