add example for converting to I/O error #82

nivkner · Sep 22, 2017

fixes #81

KodrAus · Oct 1, 2017

Thanks @nivkner! I like the idea of offering an io_error method on walkdir's Error type that returns &io::Error. It might be a bit clearer and more compact than From::from in cases where you just want to inspect part of the error.

What do you think?

nivkner · Oct 1, 2017

I think it's a good idea since you probably don't want to consume the error just to check the io::Error.

However, I did make one change: return Option<&io::Error> since there is the case where Error doesn't represent an I/O error but rather a cycle.

Unlike From where you could just construct a new error, here the return value must be a borrow, where I have nothing to borrow.

Returning something new as a borrow just results in does not live long enough error.

KodrAus · Oct 2, 2017

KodrAus reviewed Oct 2, 2017

View changes

This is looking good! Thanks @nivkner.

I'd like to know what you and @BurntSushi think about using Cow for the result of io_error instead of Option so it's always got a value. It might be that using a less common type isn't worthwhile.

KodrAus · Oct 2, 2017

src/lib.rs

+    ///
+    /// [`None`]: https://doc.rust-lang.org/stable/std/option/enum.Option.html#variant.None
+    /// [`io::Error`]: https://doc.rust-lang.org/stable/std/io/struct.Error.html
+    pub fn io_error(&self) -> Option<&io::Error> {


I wonder if this could be a Cow<'a, io::Error>? Then you could always return an error, but if it's not already an io::Error then you'll get Cow::Owned.

You should just be able to match on err.io_error().kind() then.

That would be great, except for the fact that to do that, io::Error needs to implement ToOwned to be enclosed in Cow, which it doesn't.

Ah right. That's a bit of a shame

KodrAus · Oct 2, 2017

src/lib.rs

@@ -1277,6 +1277,44 @@ impl Error {
        self.depth
    }

+    /// Inspect the underlying [`io::Error`] if there is one.
+    /// If a cycle was detected instead, [`None`] is returned.


I would just say here that if the underlying error isn't an io::Error than the result is None. My other comment might make this obsolete though.

BurntSushi · Oct 2, 2017

BurntSushi requested changes Oct 2, 2017

View changes

Overall this looks great! Thanks for helping to improve the documentation. :-)

I added a bunch of mostly stylistic nits that I hope we can fix before merging.

BurntSushi · Oct 2, 2017

src/lib.rs

+    /// [`io::Error`]: https://doc.rust-lang.org/stable/std/io/struct.Error.html
+    pub fn io_error(&self) -> Option<&io::Error> {
+       match self.inner {
+            ErrorInner::Io { path: _ , ref err } => Some(err),


You don't need to mention path here since you aren't using it. This should work: ErrorInner::Io { ref err, .. }.

BurntSushi · Oct 2, 2017

src/lib.rs

+    ///             continue
+    ///         }
+    ///     };
+    ///     println!("{}", entry.path().display());


I feel like the let entry = ... here is unnecessary. Maybe just move the println! into the match expression arm?

BurntSushi · Oct 2, 2017

src/lib.rs

+    ///
+    /// let mut it = WalkDir::new("foo").into_iter();
+    /// loop {
+    ///     let entry = match it.next() {


Could you please reformat this code so that it fits into 79 columns inclusive?

BurntSushi · Oct 2, 2017

src/lib.rs

+    /// If the underlying error isn't an [`io::Error`], [`None`] is returned.
+    ///
+    /// ```rust,no-run
+    /// use walkdir::{WalkDir};


Please remove superfluous curly braces.

BurntSushi · Oct 2, 2017

src/lib.rs

+    /// use std::path::Path;
+    ///
+    /// let mut it = WalkDir::new("foo").into_iter();
+    /// loop {


Can you use a normal for loop here?

BurntSushi · Oct 2, 2017

src/lib.rs

@@ -1277,6 +1277,44 @@ impl Error {
        self.depth
    }

+    /// Inspect the underlying [`io::Error`] if there is one.
+    /// If the underlying error isn't an [`io::Error`], [`None`] is returned.


Could the documentation be expanded to mention when this might not contain an io::Error? e.g., "In some circumstances, this Error won't correspond to an io::Error, for example, when the error was produced because a cycle was found in the directory tree while following symbolic links."

I think the docs should also mention that if the caller wants to acquire an owned io::Error, then they can convert this error into an io::Error using io::Error::from(err). And state that if this error doesn't correspond to an io::Error, then one is created.

BurntSushi · Oct 2, 2017

src/lib.rs

@@ -1277,6 +1277,44 @@ impl Error {
        self.depth
    }

+    /// Inspect the underlying [`io::Error`] if there is one.


Could you please insert a blank line here?

nivkner · Oct 7, 2017

Fixed the PR as per @BurntSushi's review.

BurntSushi · Oct 7, 2017

BurntSushi approved these changes Oct 7, 2017

View changes

Thanks!

nivkner force-pushed the nivkner:from_err branch 5 times, most recently from e511bb6 to 78d0c68 Sep 22, 2017

nivkner force-pushed the nivkner:from_err branch from 78d0c68 to a149b49 Oct 1, 2017

nivkner force-pushed the nivkner:from_err branch 3 times, most recently from d5f0822 to 9a9fb47 Oct 1, 2017

nivkner force-pushed the nivkner:from_err branch from 9a9fb47 to dde9279 Oct 2, 2017

nivkner force-pushed the nivkner:from_err branch 4 times, most recently from 50be3b8 to dbadf08 Oct 4, 2017

add `io_error` to inspect the underlying `io::Error`

Loading status checks…

8cdb7e5

nivkner force-pushed the nivkner:from_err branch from dbadf08 to 8cdb7e5 Oct 7, 2017

BurntSushi merged commit 64e50c8 into BurntSushi:master Oct 7, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

BurntSushi/walkdir

add example for converting to I/O error #82

add example for converting to I/O error #82

nivkner commented Sep 22, 2017

nivkner force-pushed the nivkner:from_err branch 5 times, most recently from `e511bb6` to `78d0c68` Sep 22, 2017

This comment has been minimized.

KodrAus commented Oct 1, 2017

nivkner force-pushed the nivkner:from_err branch from `78d0c68` to `a149b49` Oct 1, 2017

This comment has been minimized.

nivkner commented Oct 1, 2017 •

edited

nivkner force-pushed the nivkner:from_err branch 3 times, most recently from `d5f0822` to `9a9fb47` Oct 1, 2017

KodrAus reviewed Oct 2, 2017

View changes

KodrAus left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nivkner force-pushed the nivkner:from_err branch from `9a9fb47` to `dde9279` Oct 2, 2017

BurntSushi requested changes Oct 2, 2017

View changes

BurntSushi left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nivkner force-pushed the nivkner:from_err branch 4 times, most recently from `50be3b8` to `dbadf08` Oct 4, 2017

nivkner force-pushed the nivkner:from_err branch from `dbadf08` to `8cdb7e5` Oct 7, 2017

This comment has been minimized.

nivkner commented Oct 7, 2017

BurntSushi approved these changes Oct 7, 2017

View changes

BurntSushi left a comment

BurntSushi merged commit `64e50c8` into BurntSushi:master Oct 7, 2017
2 checks passed

2 checks passed

BurntSushi/walkdir

Join GitHub today

add example for converting to I/O error #82

Conversation

nivkner commented Sep 22, 2017

nivkner force-pushed the nivkner:from_err branch 5 times, most recently from e511bb6 to 78d0c68 Sep 22, 2017

This comment has been minimized.

KodrAus commented Oct 1, 2017

nivkner force-pushed the nivkner:from_err branch from 78d0c68 to a149b49 Oct 1, 2017

This comment has been minimized.

nivkner commented Oct 1, 2017 • edited

nivkner force-pushed the nivkner:from_err branch 3 times, most recently from d5f0822 to 9a9fb47 Oct 1, 2017

KodrAus reviewed Oct 2, 2017 View changes

KodrAus left a comment

This comment has been minimized.

This comment has been minimized.

nivkner Oct 2, 2017 • edited

This comment has been minimized.

This comment has been minimized.

nivkner force-pushed the nivkner:from_err branch from 9a9fb47 to dde9279 Oct 2, 2017

BurntSushi requested changes Oct 2, 2017 View changes

BurntSushi left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nivkner force-pushed the nivkner:from_err branch 4 times, most recently from 50be3b8 to dbadf08 Oct 4, 2017

nivkner force-pushed the nivkner:from_err branch from dbadf08 to 8cdb7e5 Oct 7, 2017

This comment has been minimized.

nivkner commented Oct 7, 2017

BurntSushi approved these changes Oct 7, 2017 View changes

BurntSushi left a comment

Hide details View details BurntSushi merged commit 64e50c8 into BurntSushi:master Oct 7, 2017 2 checks passed

2 checks passed

nivkner force-pushed the nivkner:from_err branch 5 times, most recently from `e511bb6` to `78d0c68` Sep 22, 2017

nivkner force-pushed the nivkner:from_err branch from `78d0c68` to `a149b49` Oct 1, 2017

nivkner commented Oct 1, 2017 •

edited

nivkner force-pushed the nivkner:from_err branch 3 times, most recently from `d5f0822` to `9a9fb47` Oct 1, 2017

KodrAus reviewed Oct 2, 2017

View changes

nivkner Oct 2, 2017 •

edited

nivkner force-pushed the nivkner:from_err branch from `9a9fb47` to `dde9279` Oct 2, 2017

BurntSushi requested changes Oct 2, 2017

View changes

nivkner force-pushed the nivkner:from_err branch 4 times, most recently from `50be3b8` to `dbadf08` Oct 4, 2017

nivkner force-pushed the nivkner:from_err branch from `dbadf08` to `8cdb7e5` Oct 7, 2017

BurntSushi approved these changes Oct 7, 2017

View changes

BurntSushi merged commit `64e50c8` into BurntSushi:master Oct 7, 2017
2 checks passed