Hide all error internals #44
Conversation
In preparation for adding a bunch more error types move the current ones out of `parse` into `error`.
tcharding
force-pushed
the
10-27-errors
branch
from
dee6b08
to
12092e5
Compare
|
Man, I forget if we decided struct errors are supposed to have EDIT: Found it, they are supposed to be: rust-bitcoin/rust-secp256k1#635 (comment) |
tcharding
force-pushed
the
10-27-errors
branch
from
12092e5
to
a05458f
Compare
src/iter.rs
Outdated
| let loh = (lo as char).to_digit(16).ok_or(HexToBytesError::InvalidChar(lo))?; | ||
| let hih = (hi as char) | ||
| .to_digit(16) | ||
| .ok_or(HexToBytesError::InvalidChar(InvalidCharError { invalid: hi }))?; |
There was a problem hiding this comment.
You can keep it short by writing ok_or(InvalidCharError { invalid: hi })?; thanks to the conversion made by ?.
There was a problem hiding this comment.
I back and forth'ed on this a bunch of times (literally added and remove code). It works sometimes and in some places it didn't so I went with uniform and wrote it out fully everywhere. I"ll have another go though.
There was a problem hiding this comment.
Oh I misspoke, you suggestion is clearly better. I was referring to the into case below.
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
This is not needed when at least one field is not pub but I could understand if someone wants to avoid us forgetting to add it in the future.
There was a problem hiding this comment.
I'm inclined to put it on every struct regardless because it makes scanning the error types quicker. However, this code is plain old sloppy, I've been not adding Copy when using non_exhaustive.
There was a problem hiding this comment.
I think naming error type *Error is much better. There are other types that need to be non_exhaustive and are not error types.
There was a problem hiding this comment.
I don't follow your comment, all errors are suffixed with Error. I'll remove the non_exhaustive from all struct errors that exist solely to hide internals. On further thought today it is strange to have unneeded non_exhaustive and there will always be private fields since that is the whole point of these types.
There was a problem hiding this comment.
Yes, they are and should continue to be. That's my point rg 'struct [A-Za-z0-9]*Error' is better than rg non_exhaustive`
|
|
||
| /// Purported hex string had odd length. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| #[non_exhaustive] |
Hide the internals of all errors by wrapping enum variant internals in structs.
tcharding
force-pushed
the
10-27-errors
branch
from
a05458f
to
af04208
Compare
|
All suggestions implemented @Kixunil except removing |
|
Please don't merge yet, I want to remove the unnecessary |
|
lol @tcharding you were just a bit too late, sorry |
|
Ha! I thought you might have started the merge process soon as you acked. I'll fix as a follow up. |
Before we do v1.0 we want to hide all the error internals so that we can modify/improve them without breaking the public API.
Please pay particular attention to the
Displaystrings because we they are considered part of the public API so we don't want to change them willy-nilly. Of note, because the enums have inner structs it was difficult to not duplicate the error message.Resolve: #42