Implement Path::try_exists #48
Conversation
Implements the `Path::try_exists` associated function mentioned in andrewhickman#46. The aliased function was stabilized in 1.63 so to preserve compatibility with older Rust versions (ci targets 1.40) a new feature flag was added.
schneems
force-pushed
the
schneems/try_exists
branch
from
72d2d51
to
93a270e
Compare
| # Adds I/O safety traits introduced in Rust 1.63 | ||
| # Adds `fs_err_try_exists` to `Path`, introduced in Rust 1.6.3 | ||
| path_try_exists = [] | ||
| # Adds I/O safety traits, introduced in Rust 1.63 |
There was a problem hiding this comment.
I'm not sure best practice on features. Other things I considered include:
Adding an all feature that loads all features:
all = ["path_try_exists", "io_safety"]
Alternatively making a feature named 1_63 that would enable all features on 1.63+.
1_63 = ["path_try_exists", "io_safety"]
| @@ -23,6 +23,8 @@ pub(crate) enum ErrorKind { | |||
| Canonicalize, | |||
| ReadLink, | |||
| SymlinkMetadata, | |||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
In order to put the method behind a feature flag, I needed to get clippy to not complain about this unused variant. Maybe there's a better way or pattern.
| @@ -1,3 +1,5 @@ | |||
| #[allow(unused_imports)] | |||
There was a problem hiding this comment.
This import is only needed when path_try_exists feature is enabled. Without the allow unused imports clippy generates a warning. Maybe there's a better way to avoid it.
In this case I think it affects the whole file, not just this one import. Which is not ideal
| #[cfg(feature = "path_try_exists")] | ||
| fn fs_err_try_exists(&self) -> io::Result<bool> { | ||
| self.try_exists() | ||
| .map_err(|source| Error::build(source, ErrorKind::FileExists, self)) |
There was a problem hiding this comment.
Weirdly enough, try_exists on Path is stabilized by fs::try_exists is not. I couldn't follow the same pattern used by the rest of the associated trait functions.
|
Instead of a feature flag, what do you think about using one of the available crates that does feature or version detection of the running compiler? I'm not sure what the other maintainers of fs-err would prefer, but I know that autocfg is a solution that would make this API available for new Rust versions without breaking older versions. |
|
I think using autocfg is a good idea too 👍 |
Per comments on andrewhickman#48 this moves from gating `try_exists` behind a feature to using `autocfg`. This is a build time library that inspects the current runtime and adds flags to the build so if a user is building with Rust 1.63 or higher then this feature will be enabled.
Per comments on andrewhickman#48 this moves from gating `try_exists` behind a feature to using `autocfg`. This is a build time library that inspects the current runtime and adds flags to the build so if a user is building with Rust 1.63 or higher then this feature will be enabled.
schneems
force-pushed
the
schneems/try_exists
branch
from
11cb168
to
4afa6f8
Compare
|
I updated to use I didn't remove the existing feature flag for |
|
Thanks! I'll publish version 2.10.0 with this change |
|
Thanks a ton for the review and for this library! |
It's best practice to use `try_exists()` instead of `exists()` when checking if a file exists or not because if there's a problem with the file (such as a permissions issue) then the check might fail even though the file is on disk. This seems like not a big deal, but can be very confusing when telling users "we didn't do X because you are missing file Y" but when they debug they see file Y and don't realize that the real problem isn't "missing file" but it's the hidden error preventing `exists()` from returning a true. This commit uses fs_err_try_exists() from fs-err library that was introduced in: andrewhickman/fs-err#48. There are a few other places where `exists` is still called, those are nested in areas that don't already return a Result so they're harder to swap.
It's best practice to use `try_exists()` instead of `exists()` when checking if a file exists or not because if there's a problem with the file (such as a permissions issue) then the check might fail even though the file is on disk. This seems like not a big deal, but can be very confusing when telling users "we didn't do X because you are missing file Y" but when they debug they see file Y and don't realize that the real problem isn't "missing file" but it's the hidden error preventing `exists()` from returning a true. This commit uses fs_err_try_exists() from fs-err library that was introduced in: andrewhickman/fs-err#48. There are a few other places where `exists` is still called, those are nested in areas that don't already return a Result so they're harder to swap.
Implements the
Path::try_existsassociated function mentioned in #46. The aliased function was stabilized in 1.63 so to preserve compatibility with older Rust versions (ci targets 1.40) a new feature flag was added.