Please correct me if there is a bigger picture that i'm missing…
There seem to be a common habit all through-out embedded-can of returning an Option instead of a Result.
For example:
impl StandardId {
...
/// Tries to create a `StandardId` from a raw 16-bit integer.
///
/// This will return `None` if `raw` is out of range of an 11-bit integer (`> 0x7FF`).
pub const fn new(raw: u16) -> Option<Self> {
if raw <= 0x7FF {
Some(Self(raw))
} else {
None
}
}
Or:
pub trait Frame: Sized {
/// Creates a new frame.
///
/// This will return `None` if the data slice is too long.
fn new(id: impl Into<Id>, data: &[u8]) -> Option<Self>;
In both cases, when:
- creating a StandardId that is more than 11-bit long, or
- creating a frame with a data payload bigger than 8 bytes
…None is returned.
However, this is not like, e.g., searching an item in a collection where it's not, thus None is rightfully returned. In both cases user-code is attempting to use illegal values, which is a logic error: therefore, an explicit Error should be returned instead.
So far it kinda works, because each such method only has one reason to fail (and return None): thus the error case is implicit, but remains "deterministic". So this may all sound like a pedantic distinction…
However, consider the following:
/// Create a new Standard Format data frame
pub fn new_standard(raw_id: u16, data: &[u8]) -> Option<Frame> {
let id = StandardId::new(raw_id)?;
Frame::new(id, data)?
}
Returning None is no longer an option (pun intended), as we can (pun intended) no longer distinguish between error cases. We're going to have to:
- use a
Result
- create two custom
Errors
- wrap each
None fail-case to return the appropriate one.
It's ugly, verbose, and over-specific in that it forces us to do boilerplate work that really is embedded-can's job, and create custom errors (e.g. IllegalId and DataLengthTooLong(u8)) that really are embedded-can's methods errors…
Therefore i'm suggesting that:
- all
Option-returning methods return a Result instead
- corresponding errors be added to
ErrorKind
Thank you!
Please correct me if there is a bigger picture that i'm missing…
There seem to be a common habit all through-out
embedded-canof returning anOptioninstead of aResult.For example:
Or:
In both cases, when:
…
Noneis returned.However, this is not like, e.g., searching an item in a collection where it's not, thus
Noneis rightfully returned. In both cases user-code is attempting to use illegal values, which is a logic error: therefore, an explicitErrorshould be returned instead.So far it kinda works, because each such method only has one reason to fail (and return
None): thus the error case is implicit, but remains "deterministic". So this may all sound like a pedantic distinction…However, consider the following:
Returning
Noneis no longer an option (pun intended), as we can (pun intended) no longer distinguish between error cases. We're going to have to:ResultErrorsNonefail-case to return the appropriate one.It's ugly, verbose, and over-specific in that it forces us to do boilerplate work that really is
embedded-can's job, and create custom errors (e.g.IllegalIdandDataLengthTooLong(u8)) that really areembedded-can's methods errors…Therefore i'm suggesting that:
Option-returning methods return aResultinsteadErrorKindThank you!