Skip to content
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

Use error feature #10

Closed
wants to merge 5 commits into from
Closed

Conversation

jonhteper
Copy link
Contributor

@jonhteper jonhteper commented Dec 28, 2022

The usecase what I'm resolving is the "error propagation", usecase derive from error types design in Rust.

Error types design in Rust

In Rust normally use the Result<T, E> type, and propagate error in functions using the try-operator ?, but this type is too flexible with the E generic. The E type not even needs to implement the Display trait; and for this reason not always can use the Box<dyn Error> like common place.

For solve this complication, the developers use some guidelines for Error types design, for example the majory implements the Display and Debug traits, and use common ways like "enum style" or "struct style", and use the From trait for converting error types.

Struct Style

pub enum ErrorKind {
   Malformed,
   MissingHeader,
   UnknownField,
   IOError,
   VarError
}


pub struct MyError {
   kind: ErrorKind,
   message: String,
   source: Option<Box<dyn Error>>,
}

// Example based in https://nrc.github.io/error-docs/error-design/error-type-design.html#single-struct-style

Enum Style

pub enum MyError {
   Malformed(MalformedRespError),
   MissingHeader(String),
   UnknownField(String),
   VarError(std::env::VarError),
   IO(std::io::Error),
}

pub struct MalformedRespError {
// ...
}

// Example based in https://nrc.github.io/error-docs/error-design/error-type-design.html#enum-style

*In reality, most programmers use thiserror for error convertions.

Of course the above only implementing if you considered important identify the errors individually, which it may not important in little applications in user side, but in crates, enhances the develop experience.

Just now, the NonEmptyString constructors returns a empty String that difficult the From trait implementation, because its necessary discriminate it to others Result<T, String> that provides explicit information for error handling. And the justification for return the input if this is empty, just cover an extreme edge case and forget the common cases.

In this PR I use a Cargo feature to enable the ErrorEmptyString type, but some different answers that I propose:

  • Return a String with error description, for example Err("Error: zero-value string".to_string()).
  • Return a new error type, for example ErrorEmptyString (in this PR) or other.
  • Use std::io::ErrorKind::InvalidInput .
  • Use std::io::Error, for example: std::io::Error::new(std::io::ErrorKind::InvalidInput, "zero-value string")
  • Use Option in NonEmptyString::new and implement one of the above answers.

@MidasLamb
Copy link
Owner

The reason I opted for a Result with the error variant being a String, is because the length of a String could be 0, however it could have a very large capacity.
While for NonZeroU8 for example we know the total size in memory when attempting construction, we know that recreating it is cheap and quick.
However for the String it could potentially be backed by a big heap allocation that we would have to free when dropping the passed in String, which might have to be reallocated later on again because of string manipulation.

That being said, I do see the ergonomics improvement you're referring to, but rather than having the same function change signature based on a feature flag (which I don't think follows the additive requirement of features), we could maybe create a new constructor behind a feature flag which would provide the more descriptive error.

I'm going to let this simmer for a bit before I decide which direction I want to take with this

@jonhteper
Copy link
Contributor Author

Now, I'm understad more why is important to return the String in the constructor. And I belive to I was found a solution to implement both constructors: using FromStr trait. I just open a new PR.

@MidasLamb
Copy link
Owner

MidasLamb commented Aug 26, 2023

Superceded by #12

@MidasLamb MidasLamb closed this Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants