-
Notifications
You must be signed in to change notification settings - Fork 32
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
file.find() #405
file.find() #405
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #405 +/- ##
==========================================
- Coverage 71.95% 71.39% -0.56%
==========================================
Files 112 113 +1
Lines 8729 8800 +71
==========================================
+ Hits 6281 6283 +2
- Misses 2339 2408 +69
Partials 109 109 ☔ View full report in Codecov by Sentry. |
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.
Couple changes
} | ||
#[cfg(windows)] | ||
{ | ||
if permissions == 0 && metadata.readonly() { |
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.
Could be cleaner to switch to a match.
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.
Problem is that for unix I'm making calls to a permissions trait in PermissionsExt
, so if we don't do this then the Windows version will try and compile with the use of the unix-only trait and 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.
Couldn't it still be bound in the #[cfg(windows)]
?
I'm thinking
#[cfg(windows)]
{
match permissions {
0 => {
if metadata.readonly() { return Ok(false) }
}
1 => {
if !metadata.readonly() { return Ok(false) }
}
_ => {
return anyhow::anyhow!("Windows only supports readonly (0) or not-readonly (1)")
}
}
}
} | ||
} | ||
} | ||
if let Some(modified_time) = modified_time { |
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.
Outside the scope of this PR but in the future is it possible to add newer than or older than?
Seeing this I realized it'll be be rare files match a specific second.
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.
Yeah that was what I was curious about with these. Do we want older/newer to be the default action?
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.
If you want to make the changes in this PR i would:
- Remove the modified time and created time fields
- Replace modified time with a modified_before and modified_after option
- Replace created time with a created_before and created_after option
This would make it possible to time bound your search for things created within a specifir and or modified in a specific range.
This looks good to me. |
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.
Works for me.
What type of PR is this?
/kind documentation
/kind feature
/kind eldritch-function
What this PR does / why we need it:
Adds way to search given directory for file matching criteria.
Please give feedback on current implementation. Imo its kinda rough.
Which issue(s) this PR fixes:
Fixes #66