-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
rm
: refactor prompt_file
, issue #5345
#5356
Conversation
Did you try moving this into a function? |
No I only tried to address the nesting depth of the control flow mostly with early returns. Since I orientated myself on tertsdiepraam comment in rm: refactor prompt file, issue #5345. |
if you don't mind trying, yeah, moving it to a function could help to make the code more readable :) |
What is the right procedure now closing the pull request and opening a new one? |
@terade just add on to this one and rename it! For some pointers: look into |
rm
: refactor prompt_file
, issue #5345
ce90c48
to
f18e898
Compare
Addressing issue uutils#5345. Introduce new helper function: prompt_file_permission_read_only
rm
: refactor prompt_file
, issue #5345rm
: refactor prompt_file
, issue #5345
Should I apply the lint suggestion (it isn't related to my code) or would that lead to confusion, on why an unrelated file has been touched? |
src/uu/rm/src/rm.rs
Outdated
fn prompt_file_permission_readonly( | ||
path: &Path, | ||
metadata_or_err: Result<Metadata, std::io::Error>, | ||
) -> bool { | ||
match metadata_or_err { |
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.
Passing a Result
to the function looks a bit odd to me. I think it would be cleaner to retrieve the metadata inside the function:
fn prompt_file_permission_readonly( | |
path: &Path, | |
metadata_or_err: Result<Metadata, std::io::Error>, | |
) -> bool { | |
match metadata_or_err { | |
fn prompt_file_permission_readonly(path: &Path) -> bool { | |
match fs::metadata(path) { |
} | ||
} | ||
prompt_file_permission_readonly(path) |
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.
Good catch, I didn't notice this simplification :)
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.
Should I still leave it in its own function or move it into prompt_file?
(and thanks :))
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's fine as it is.
Thanks for your PR! |
Refactor
prompt_file
, add helper function.