BurntSushi / walkdir

Rust library for walking directories recursively.
The Unlicense
1.21k stars 106 forks source link

Implementing From for my custom error #157

Closed oren0e closed 2 years ago

oren0e commented 2 years ago

I have a similar structure to walkdir::Error where I have a public struct and (also public, unlike in walkdir) an enum. One of the variants in this enum represents IO errors. I found myself trying to implement impl From<walkdir::Error> for MyError. I wanted to do something along the lines of:

    fn from(walk_err: walkdir::Error) -> MyError {
        let kind = match walk_err {
            walkdir::Error {
                inner: walkdir::ErrorInner::Io { ref err, .. },
                ..
            } => err.kind(),
            walkdir::Error {
                inner: ErrorInner::Loop { .. },
                ..
            } => std::io::ErrorKind::Other,
        };
        std::io::Error::new(kind, walk_err).map_err(MyErrorInner::IOError)?
    }

The problem is that walkdir::ErrorInner is private. What can I do? I only want to be able to convert between the errors.

BurntSushi commented 2 years ago

This kind of seems like an XY problem here. What problem are you trying to solve at a high level?

The reason why I ask is because I can think of at least two alternatives here other than walkdir exposing its error representation, but it's not clear to me why those alternatives are insufficient. Those alternatives, given the info I have, I believe are:

oren0e commented 2 years ago

@BurntSushi You are right that the argument of a dependency was in my mind when I thought about whether to expose the inner error or not, but actually now I'm confused because from a discussion I had earlier with a teammate I couldn't pin point the problem with that. My thought was that if I expose it, then if some user will match against an error variant that has thiserror's #[from] with some other external crate error - this can be a problem in case the external error changed. But I'm not sure anymore since I don't think I'm writing a library that other programmers will use, I'm writing a web service (with rocket, reqwest and mongo in the back). This is slightly off topic but why did you decide on the "private inner and public error" structure in walkdir?

BurntSushi commented 2 years ago

This is slightly off topic but why did you decide on the "private inner and public error" structure in walkdir?

I think it's possible the framing of this question might be what's leading you astray. In general, the question should not be, "why did you make this private?", but rather, "why did you make this public?" When I start designing an API, you start with an empty canvas. For every new public API item you introduce, you should have a reason for it. This is because the public API defines the substrate on which you communicate with others. If you change that API, then you have to communicate those changes to everyone else. Particularly if it's a breaking change. This results in churn.

But you make changes to things that aren't public, then there is no communication overhead. You are free to make those changes at whatever whim you wish.

So that's why. I don't make things private for a reason. I make things public for a reason. If there is no good reason for making it public, then it keeps its default state: private.

If you don't care about walkdir being a public dependency, then yeah, it might be good enough to just expose it.

You also didn't address my second alternative though. You can use the public API of walkdir::Error to extract all of its information and stuff it into your own type if you so desire. (And this is likely what I would recommend I think.)

oren0e commented 2 years ago

Thanks a lot for the detailed answer! I'll give the second alternative you've proposed a try. Closing the issue for now.