Using consistent place for Error types

[JelteF]

Some derives require an error type. Right now three of those errors are contained in the ops module and three others are simply in the root. I think before 1.0 we should make it consistent from where we export those error types.

I can see three options:

1. Export them all from an error module
2. Export them all from the root
3. Export them from differently named modules based on the derives that they are used by, e.g. ops, convert, unwrap and str

My personal preference would go to simply all exporting them from the root (option 2). But I think option 1 is also acceptable. Option 3 add needless complexity in my opinion.

@tyranron @ilslv what do you think?

[tyranron]

@JelteF I'd prefer the option 3. It's easier to understand the code and operate on it when the modules have a logical separation and irrelevant stuff is not mixed together. Don't really like "pile of stuff" modules.

[JelteF]

For 3 the main reason I don't like it is, because it would mean having 3 modules with just a single item in them. And that just seems like useless indirection. i.e. if you're making groups so small that there's only one thing in the group, you might as well not group at all. The three errors in question would be:

1. str::FromStrError
2. convert::TryIntoError
3. unwrap::TryUnwrapError

[tyranron]

@JelteF I don't see any trouble in small modules. For me, it's easier to operate on separate small units, so irrelevant stuff doesn't bring any unnecessary cognitive load into the scope. And it's easier to not being messed up with cfgs: one module - one cfg feature condition.

[JelteF]

I also don't see a trouble with small modules, but I do see a problem with modules that only contain one item.

If you have the following code.

use some::module::*

I don't think there's a cognitive load difference between it bringing 3 structs into scope or it bringing 3 other modules into scope, which all contain a single struct. The only difference is that the module approach adds some extra indirection, because now you need to know both the name of the struct and the module that it's in before you can use it. To me the whole point of modules is to group related things together, and if you have modules with only a single item then you're not grouping things together.

[tyranron]

@JelteF

because now you need to know both the name of the struct and the module that it's in before you can use it.

I'm not against re-exports, if this is the case.

but I do see a problem with modules that only contain one item.

My point is that we have multiple items, despite having a single type there. Like this:

use core::fmt;

/// Error of parsing an enum value its string representation.
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub struct FromStrError {
    type_name: &'static str,
}

impl FromStrError {
    #[doc(hidden)]
    #[must_use]
    #[inline]
    pub const fn new(type_name: &'static str) -> Self {
        Self { type_name }
    }
}

impl fmt::Display for FromStrError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "Invalid `{}` string representation", self.type_name)
    }
}

#[cfg(feature = "std")]
impl std::error::Error for FromStrError {}

So here, a type and its impls are grouped together under a single module, having a single #[cfg(feature = "from_str")] atop.

Mixing it together with other errors and their impls inside a single module, would require placing those #[cfg(feature = "...")]s granularly and carefully for each impl, use and/or other definitions, polluting the code with repeated stuff and increasing cognitive load when reading it and changing it (you should not be carefull to not mess up imports, for example).

[JelteF]

Okay, so I think we were talking about different things. I think the internal structure that we currently have is fine. Having 1 error per file is okay, and indeed (imho) preferable to putting them all in the same file. I was trying to discuss the way we export them to our users. Right now we don't do this consistently. If you look at the docs you will see this:

Some errors exported from the root module, and an ops module exported from the root module, that contains a few more errors. That's the consistency issue I wanted to address by opening this issue.

[tyranron]

@JelteF regarding re-exporting for users I'd prefer the option 2 then:

2. Export them all from the root