The postgres::Error is hard to work with

[fschutt]

1. The Error struct is missing an as_tls() function. There is no way to get the inner type of the TLS error, even though the library internally does have this information.
2. Is there any good reason why the internal error enum private? Why does the error handling use a Error(Box<ErrorKind>) (and ErrorKind is simple enum) instead of just having the ErrorKind as Error in the public API? It completely prevents pattern matching:

let mut err_str = format!("Unknown error, probably SSL verification failed"); // default case because no as_tls() function

if let Some(e) = e.as_connection() {
    err_str = format!("Could not authenticate the connection when connecting to the database.\n\n{}", e);
}

if let Some(e) = e.as_db() {
    err_str = format!("Could not connect to the database.\n\n{}", e);
}

if let Some(c) = e.as_conversion() {
    err_str = format!("Field conversion error (internal data type did not match the SQL query data type).\n\n{}", c);
}

if let Some(c) = e.as_io() {
    err_str = format!("Database read / write error:\n\n{}", c);
}

instead of:

let err_str = match e {
    ConnectParams(e) => { /**/ },
    Tls(e) => { /**/ },
    Db(e) => { /**/ },
    Io(e) => { /**/ },
    Conversion(e) => { /**/ },
};

All that the overcomplicated postgres::Error is doing is manually implementing pattern-matching. But now I can't match on two errors at the same time, ... why are you doing this??? If you want to prevent outsiders from creating a ErrorKind, that in itself is fine, but you could just make a function that returns a read-only reference to the inner error kind, not prevent pattern matching completely.

I can't use the .description() and .cause() methods, since these error strings have to be localized in the real code. If I had an enum, I could easily do this, as this is standard error case for Rust crates.

3. Why is everything Box<Error + Sync + Send> when all you are doing in the library is putting Strings into that Box? Why isn't this an enum - you've simply erased all type information and now all errors are strings. Just use enums for returning errors. At the very least you could use String instead of Box<Error + Sync + Send>.

4. Why is everything in this library an io::Error::Other("String")? Things like parsing errors don't belong in the category of IO errors.

[fschutt]

None of the library internal structure require that you return errors as Strings (and therefore, do unnecessary dynamic allocation and type erasure). Why the hell are errors just strings, but presented as Box<Error + Send + Sync> with the library just doing "error string".to_owned().into()? This is what the Display trait is for!

Lots of code in this library looks like this:

return Err(io::Error::new(
    io::ErrorKind::InvalidData,
    "invalid message length",
));

This is what 90% of errors look like in this library, it's very bad style. Why does a simple String need to be wrapped in an IO error? Why does it need to be a String in the first place? It could just be:

return Err(ParseError::InvalidMessageLength);

impl Display for ParseError { 
    /* snip */
    match self {
        InvalidMessageLength => write!(f, "invalid message length");
    }
}

Why is everything in this library just "errors are strings"? No pattern matching, no stability, English-only, no exhaustive matching ... why???

[fschutt]

Sorry if my previous messages were a bit rude, I was in a bad mood. I'm working on a PR with strongly-typed errors, then you can see what I mean by "doing it correctly".

If you are just doing the io::Error thing because of Tokio, don't. You can always convert an error into an io::Error and stringify it later on, but you can never go back from a stringified error to a strongly typed one.

[sfackler]

Don't be an asshole.

The Error struct is missing an as_tls() function.

Yep, this is an oversight, and should be added.

Is there any good reason why the internal error enum private?

This style of error is not uncommon. It's used in serde_json and hyper for example. Providing direct access to the backing enum overly constrains the implementation. hyperium/hyper#1131 has more background.

Why is everything in this library an io::Error::Other("String")? Things like parsing errors don't belong in the category of IO errors.

In which library? io::Error is not the return type of anything other than postgres-protocol. Errors are only returned from that if either Postgres is broken or we are. Why does a user care about the distinction between the database returning a 5 byte buffer when we're expecting a 4 byte buffer, and returning a 3 byte buffer?

Why is everything Box<Error + Sync + Send> when all you are doing in the library is putting Strings into that Box?

"Everything" is not a Box<Error>. The portions of the library that are designed to be externally extensible and return errors work with Box<Error>. I am not putting strings in those boxes either, but rather concrete error types.

Why isn't this an enum - you've simply erased all type information and now all errors are strings. Just use enums for returning errors.

What does that enum look like? What is the set of all possible TLS errors? What is the set of all possible conversion errors?

No, I have not erased all type information. You can downcast Box<Error> types.

This is what 90% of errors look like in this library, it's very bad style.

You are aware that postgres-protocol is only one part of this library, right?

no exhaustive matching ... why???

"exhaustive matching" means "no ability to extend without making a breaking change".

If you are just doing the io::Error thing because of Tokio, don't.

What does Tokio have to do with any of this?

[gustav3d]

Having access to the enum error::Kind would certainly make for more straight forward usage.
Its in fact a bit odd that its hidden.