Handle ErrorKind NotFound instead of file.exists() #834

BProg · 2020-09-22T22:15:11Z

@charlespierce I am ok with any kind of comments. I would like to know if this is the correct direction you want to take in regards to issue 823.

I have changed only the places that do an open operation on FS, and left alone other operations on FS like rename or delete
There may be some missing places that I could not find

Extend the Fallible struct with 2 new functions to help with converting the NotFound Err into Ok Add new functions to the VoltaError struct to be able to determine if the inner source is of type io::ErrorKind::NotFound

charlespierce

Thanks for this @BProg! This looks like it cleans up all of the spots where we were doing the exists() + open() anti-pattern. My comments are mainly around reducing boilerplate and making things more generic.

In general, I think we should try to have the checks for NotFound as close as possible to the IO operation that actually caused them. That will help reduce the amount of times we need to check in separate locations for the same condition, and make it easier to maintain going forward.

crates/volta-migrate/src/v2.rs

crates/volta-core/src/error/mod.rs

charlespierce · 2020-09-23T18:17:37Z

crates/volta-core/src/error/mod.rs

+pub trait AcceptableErrorToValue<T> {
+    fn accept_error_as_value_if<F1, F2>(self, accept_if: F1, as_value: F2) -> Fallible<T>
+    where
+        F1: FnOnce(&VoltaError) -> bool,
+        F2: FnOnce() -> T;
+}
+
+pub trait AcceptableErrorToDefault<T>: AcceptableErrorToValue<T>


I haven't worked all the way through it, but I think a lot of this machinery could be handled with a helper function and the built-in Result::or_else method, which I think would simplify it a fair amount.

crates/volta-core/src/project/mod.rs

crates/volta-core/src/run/binary.rs

crates/volta-core/src/tool/package/mod.rs

crates/volta-core/src/tool/package_global/uninstall.rs

crates/volta-migrate/src/v1.rs

charlespierce · 2020-09-23T18:40:17Z

crates/volta-migrate/src/v2.rs

+    let platform_json =
+        match read_to_string(platform_file).with_context(|| ErrorKind::ReadPlatformError {
+            file: platform_file.to_owned(),
+        }) {
+            Err(error) => {
+                if error.is_not_found_error_kind() {
+                    return Ok(());
+                } else {
+                    return Err(error);
+                }
+            }
+            Ok(json) => json,
+        };


I think this can be simplified a little by removing the call to with_context and instead making it part of the error branch:

let platform_json = match read_to_string(platform_file) { Err(error) => { if error.kind() == io::ErrorKind::NotFound { return Ok(()); } else { return Err(VoltaError::from_source(error, ErrorKind::ReadPlatformError { file: platform_file.to_owned() })); } } Ok(json) => json, }

Simplified by using .or_else(ok_if_not_found)

crates/volta-migrate/src/v1.rs

charlespierce

This looks so much simpler than the path.exists() checks, thanks @BProg! The remaining comments are primarily around style and cleaning up some clippy warnings, nothing major.

charlespierce · 2020-09-24T17:00:38Z

crates/volta-core/src/tool/package/metadata.rs

+            }
+            Ok(config_src) => RawBinConfig::from_json(config_src)?
+                .try_into()
+                .map(|config| Some(config)),


Clippy raises a warning here: You actually don't need the closure |config| Some(config), you can instead do this with:

Suggested change

.map(|config| Some(config)),

.map(Some),

charlespierce · 2020-09-24T17:08:21Z

crates/volta-core/src/tool/package/mod.rs

+use crate::{
+    error::{Context, ErrorKind, Fallible},
+    fs::ok_if_not_found,
+};


To keep the diffs simpler, we've generally tried to keep our use statements to single modules at a time. So instead of grouping the submodules together like this, we would have 2 separate lines:

use crate::error::{Context, ErrorKind, Fallible}; use crate::fs::ok_if_not_found;

(With the appropriate ordering provided by cargo fmt.

charlespierce · 2020-09-24T17:10:09Z

crates/volta-core/src/tool/package/mod.rs

-        true
-    } else {
-        // there is no package config - check for orphaned binaries
+    let remove_orphan_binaries_if_any = || -> Fallible<bool> {


Since we aren't re-using this function anywhere else, I think it would be slightly clearer if this block was included in the match arm below, instead of creating a closure and then calling it.

charlespierce · 2020-09-24T17:13:23Z

crates/volta-core/src/tool/package/mod.rs

-    }
+    dir_entry_match(&bin_config_dir, |entry| {
+        let path = entry.path();
+        if let Ok(Some(config)) = BinConfig::from_file_if_exists(path) {


Very minor, but since we're only matching on Ok(Some(_)) here, I think we can leave this line as it was (since it amounts to the same thing):

Suggested change

if let Ok(Some(config)) = BinConfig::from_file_if_exists(path) {

if let Ok(config) = BinConfig::from_file(path) {

charlespierce · 2020-09-24T17:14:19Z

crates/volta-core/src/tool/package/mod.rs

+        };
+        None
+    })
+    .or_else(ok_if_not_found)


This is so much cleaner than the if exists() { } else { Ok(vec![]) } blocks! 🎉

charlespierce · 2020-09-24T17:17:54Z

crates/volta-migrate/src/v2.rs

+use volta_core::{
+    error::{Context, ErrorKind, Fallible, VoltaError},
+    fs::ok_if_not_found,
+};


Same comment here about keeping the imports separated. It also looks like we actually have a line with use volta_core::fs::{...}, so the additional import can be added to that line instead of a separate one.

charlespierce · 2020-09-24T17:18:48Z

crates/volta-migrate/src/v1.rs

+            remove_file(&old_volta_bin)
+                .or_else(ok_if_not_found)


Instead of directly doing remove_file().or_else(), there actually is a helper in volta_core::fs named remove_file_if_exists, so we should re-use that (it internally does the same thing since it's a common pattern).

charlespierce · 2020-09-24T17:19:08Z

crates/volta-migrate/src/v1.rs

+            remove_file(&old_shim_bin)
+                .or_else(ok_if_not_found)


Same here about using remove_file_if_exists instead of remove_file().or_else()

charlespierce · 2020-09-24T17:19:28Z

crates/volta-migrate/src/v2.rs

+        remove_file(old_layout_file)
+            .or_else(ok_if_not_found)


Here as well with remove_file_if_exists

charlespierce · 2020-09-24T17:25:40Z

crates/volta-migrate/src/v2.rs

+    let platform_json = read_to_string(platform_file)
+        .or_else(ok_if_not_found)
+        .with_context(|| ErrorKind::ReadPlatformError {
+            file: platform_file.to_owned(),
+        })?;


This happens to work because the following line Platform::from_json handles an empty string, but I'm not sure if we should be relying on that behavior. Instead, I think this can be a little more explicit about handling the NotFound error (similar to the implementation of from_file_if_exists above):

let platform_json = match read_to_string(platform_file) { Ok(json) => json, Err(error) => if error.kind() == io::ErrorKind::NotFound { return Ok(()); } else { return Err(VoltaError::from_source(error, ErrorKind::ReadPlatformError { ... })); } };

charlespierce

This is awesome, thanks for the help @BProg! ⚡ 🎉

BProg added 3 commits September 23, 2020 00:52

Handle ErrorKind NotFound instead of file exists

fe7ab44

Extend the Fallible struct with 2 new functions to help with converting the NotFound Err into Ok Add new functions to the VoltaError struct to be able to determine if the inner source is of type io::ErrorKind::NotFound

Improvement

566a64b

Cleanup

81929bd

charlespierce requested changes Sep 23, 2020

View reviewed changes

Apply changes per @charlespierce's comments

5cd63bf

BProg requested a review from charlespierce September 24, 2020 16:30

charlespierce requested changes Sep 24, 2020

View reviewed changes

Apply requested changes round 2

4be5f4c

charlespierce approved these changes Sep 26, 2020

View reviewed changes

charlespierce merged commit 9d55f4c into volta-cli:main Sep 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle ErrorKind NotFound instead of file.exists() #834

Handle ErrorKind NotFound instead of file.exists() #834

BProg commented Sep 22, 2020

charlespierce left a comment

charlespierce Sep 23, 2020

charlespierce Sep 23, 2020

BProg Sep 24, 2020

charlespierce left a comment

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce Sep 24, 2020

charlespierce left a comment

	if let Ok(Some(config)) = BinConfig::from_file_if_exists(path) {
	if let Ok(config) = BinConfig::from_file(path) {

Handle ErrorKind NotFound instead of file.exists() #834

Handle ErrorKind NotFound instead of file.exists() #834

Conversation

BProg commented Sep 22, 2020

charlespierce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlespierce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlespierce left a comment

Choose a reason for hiding this comment