-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shared error module #164
Shared error module #164
Conversation
65c8f39
to
4cf512f
Compare
How much more work do you think is needed for this? I'm wondering if there's merit in reviewing and merging what we have and making future improvements in later PRs. |
I was planning to say today that it is better to review now before it gets too bloated. I will add one decode sample and maybe two tests and I think it can be then reviewed. Doing them right now. The main issue is on categorising the errors, since most of the codec-specific ones will be empty. I guess we should hide the visibility of those. I know that many if not all of the UPER errors will be re-used by OER and even BER could re-use many of them one day if it supports constraints and also if the general error handling will be improved, so most are generic errors. But I can adjust them after you have reviewed. So, I think we should decide, whether
|
Also I noticed that decoding strings on release build do not throw any errors, and on debug mode they are only debug asserts. There will be performance impact if we check for every character whether the string type is valid, and on the other hand, should we be very strict on that? Seems like original idea is, that on decoding side it is okay to get invalid strings. We could also check validity after the whole string is decoded, but string can be also very long and you know then only in the end that it is not valid. Maybe this is better to leave some other PR. |
Could you clarify this part?
I think we should reverse course on this, and make it error in release mode, we could add an unchecked method if it's a significant performance impact. |
I think it is better to review now if I need to do some major changes. Everything but correct and complete error categorisation should be there. |
Thank you for your PR! |
I guess everything was good 😄 It is easy to move errors later on to better categories. I can finish OER now and it might bring some clarity as well. |
Yeah nothing stood out to me as needing to be changed. I saw some docs on some errors that need to be improved but they were there before your PR. |
Hi,
I think I need a bit help on deciding how we should do this.
Currently errors have been combined and they follow the similar way than existing errors in PER.
I was not sure how we should support
snafu
.Currently errors try to create stable API by allowing calling the errors either by calling function or calling
snafu
enum with.into()
. Constructing the whole enum with regular style is still possible, but I tried make this simpler by adding functions similarly than in PER.snafu
is abstracted mostly away in this way, which lets as to change the internals of the error, if needed in the future.This makes it harder to use
.context()
method ofsnafu
, but now every error has backtrace included, so maybe it is not needed.Currently the error module is constructed from the following structs/enums
EncodeError
EncoreErrorKind (snafu)
and
DecodeError
DecoreErrorKind (snafu)
Also, what should we do with the
codec.rs
now, since the all error types are same format. Do we need separate Error type for every codec? Existing way of adding codec context for every error should still work, but I am not sure how useful it is.