We should have disctinct error values for parsing and building

[Trolldemorted]

Imho using one error type for everything does not represent reality correctly. We should have one error type for each semantic group of errors, and the types should have distinguishable and straightforward names. E.g. if I am building a pack file I should never receive an invalid header error, because our api must prevent me from attempting that at compile-time.

[Frodo45127]

The error is supposed to be something the lib can return to whatever program it uses. That means it's something the person that makes that program has to deal with. It's far easier to say "So this lib has a custom error type, with X variants. And I'm sure that, if something fails in the lib, I'm going to receive that error. So I can just use it, or implement From<> for it to my custom error type". With different error types you don't really archive anything other that having extra enums that not only you have to use, but others will have to manually use depending on the operation.

From an external user perspective, one doesn't care if there was X or Y error enum coming from the lib. What it cares about it's what variant the error throws at him, as that's what's going to tell him what happen.

I've seen both approaches used in the wild (serde_json uses one error, the csv crate use multiple) and in my opinion, for someone using your lib, it's easier to use the single enum method instead of the 2 enums + an empty struct that was before the error patch.

[Trolldemorted]

The error is supposed to be something the lib can return to whatever program it uses. That means it's something the person that makes that program has to deal with. It's far easier to say "So this lib has a custom error type, with X variants. And I'm sure that, if something fails in the lib, I'm going to receive that error. So I can just use it, or implement From<> for it to my custom error type". With different error types you don't really archive anything other that having extra enums that not only you have to use, but others will have to manually use depending on the operation.

If I am matching tw_packlib's errors, I have to create a match arm for every enum variant, or use `and do nasty stuff like unreachable!() there. By using one big error type where most of the variants will be never used you are encouraging the usage of broad-arms or genericFromimplementations, which you imho really should not. If we introduce a new error variant, all users upgrading should be forced by the compiler to handle the new error variant appropriately, but if they are inclined to use` they will never notice the API has changed.

From an external user perspective, one doesn't care if there was X or Y error enum coming from the lib. What it cares about it's what variant the error throws at him, as that's what's going to tell him what happen.

I definitely care about receiving an enum where all variants are valid candidates! Otherwise your API is inconsistent: You declare that a certain set of errors can occur, where in reality only a subset can. Inconsistencies in your API are never a good thing.

I've seen both approaches used in the wild (serde_json uses one error, the csv crate use multiple) and in my opinion, for someone using your lib, it's easier to use the single enum method instead of the 2 enums + an empty struct that was before the error patch.

In the standard lib I have so far seen disctinct values only (e.g. take Sender and Receiver and their SendError, RecvError and TryRecvError enums), and the stdlib is the best rust code I have read so far. We should make make things as simple as reaonably possible, but not simpler.

Also, calling our error type just "Error" is bad because you cannot import it with std::error::Error in the same file without custom renaming, and other libs might chose the same name, hence we should switch to unique names to avoid name collisions.

[Frodo45127]

If I am matching tw_packlib's errors, I have to create a match arm for every enum variant, or use and do nasty stuff like unreachable!() there. By using one big error type where most of the variants will be never used you are encouraging the usage of broad -arms or generic From implementations, which you imho really should not. If we introduce a new error variant, all users upgrading should be forced by the compiler to handle the new error variant appropriately, but if they are inclined to use they will never notice the API has changed.

Errr.... no. If you receive an error from a lib, usually you do one of three things: report the error (which a Display implementation in the lib or a string inside the variants can fix), check if it failed or not (which you can do with if x.is_err() {}), or turn it into your own error and do one of the options before. Usually, you're not interested in manually match the specific type of error except for very specific use cases. Taking RPFM for example, the only places where specific errors variants are checked are in From implementations and in thread communication, and the second one its probably going to go away because in two months I havent received any report related with those. So no. In reality, unless you want to do some very specific stuff depending on what of multiple error variants you can receive, you should not even use a match for the errors you receive.

If we introduce a new error variant, all users upgrading should be forced by the compiler to handle the new error variant appropriately, but if they are inclined to use _ they will never notice the API has changed.

Don't see any problem here, other than people should read changelogs for breaking changes of things they're using. If they don't, is their responsability, not ours.

In the standard lib I have so far seen disctinct values only (e.g. take Sender and Receiver and their SendError, RecvError and TryRecvError enums), and the stdlib is the best rust code I have read so far. We should make make things as simple as reaonably possible, but not simpler.

And there is a reason why the failure lib is so popular. Unless you want to do something specific with those errors (which btw usually you have to process in some way, because the error message they return is not understandable for a normal user) you just check if an operation failed and return an error message to the user. Now go up all the way to, for example, a function in RPFM. There, you can have an error due to serde_json, multiple error types due to csv, io errors, and so on. So the "sane" thing is to just put them all together in one place. If a lib just return you an error with defined variants, like serde_json, you do a From for it. And if a lib have multiple error types like this one had and a From isn't really feasible, like the csv have, I'll just make my own type that say "it failed when doing X" and ignore the error altogether.

You can check at std::io::Error, how even there they recommend to not match again their variants, because they may change in the future. Error management is already heavy in rust. Add to that having to deal with multiple error types with duplicate variants (like the IOError) and it starts to become painful.

Also, calling our error type just "Error" is bad because you cannot import it with std::error::Error in the same file without custom renaming, and other libs might chose the same name, hence we should switch to unique names to avoid name collisions.

use std::error;
let x: error::Error;

Done. I've already done this for a lot of parts with the same names in the Qt lib, there is no real problem with it. And it's done that way in serde_json, csv, hyper,.... even inside the std itself, there is multiple Error types so there is no real problem with it.

[Trolldemorted]

Usually, you're not interested in manually match the specific type of error except for very specific use cases.

That depends on the user - I usually always match the exact error, but I am using rust for low-level stuff mostly. Since rpfm and tw_unpack both don't match it it is fine, I guess.