Skip to content
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

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

terade
Copy link
Contributor

@terade terade commented Oct 4, 2023

Refactor prompt_file, add helper function.

@cakebaker cakebaker linked an issue Oct 4, 2023 that may be closed by this pull request
@cakebaker cakebaker changed the title Refactor prompt file, issue #5345 rm: refactor prompt file, issue #5345 Oct 4, 2023
@sylvestre
Copy link
Contributor

Did you try moving this into a function?

@terade
Copy link
Contributor Author

terade commented Oct 4, 2023

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.
Should I additionally use a helper function?

@sylvestre
Copy link
Contributor

sylvestre commented Oct 4, 2023

if you don't mind trying, yeah, moving it to a function could help to make the code more readable :)

@terade
Copy link
Contributor Author

terade commented Oct 4, 2023

What is the right procedure now closing the pull request and opening a new one?

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Oct 4, 2023

@terade just add on to this one and rename it!

For some pointers: look into if let and let else, which would be really helpful here.

@terade terade changed the title rm: refactor prompt file, issue #5345 rm: refactor prompt_file, issue #5345 Oct 5, 2023
Addressing issue uutils#5345.
Introduce new helper function: prompt_file_permission_read_only
@tertsdiepraam tertsdiepraam changed the title rm: refactor prompt_file, issue #5345 rm: refactor prompt_file, issue #5345 Oct 6, 2023
@terade
Copy link
Contributor Author

terade commented Oct 8, 2023

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?
Sorry again for the extra work created by me.

@cakebaker
Copy link
Contributor

@terade no, this is not necessary. It is already fixed in #5360

And there's no need to apologize, it's a legitimate question :)

Comment on lines 528 to 532
fn prompt_file_permission_readonly(
path: &Path,
metadata_or_err: Result<Metadata, std::io::Error>,
) -> bool {
match metadata_or_err {
Copy link
Contributor

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:

Suggested change
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)
Copy link
Contributor

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 :)

Copy link
Contributor Author

@terade terade Oct 17, 2023

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 :))

Copy link
Contributor

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.

@cakebaker cakebaker merged commit 7a60819 into uutils:main Oct 18, 2023
42 of 45 checks passed
@cakebaker
Copy link
Contributor

Thanks for your PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rm: reduce nesting in prompt_file
4 participants