Skip to content

Implement specific errors for parser types #152

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

Open
mara-schulke opened this issue Oct 26, 2023 · 0 comments
Open

Implement specific errors for parser types #152

mara-schulke opened this issue Oct 26, 2023 · 0 comments
Assignees
Labels
complexity::low Issues or ideas with a low implementation cost component::cli Everything related to the buffrs cli docs::crate Improvements or additions to documentation priority::high Urgent change or idea, please review quickly type::refactoring Changing the inner-workings of buffrs

Comments

@mara-schulke
Copy link
Contributor

          This is a nitpick, feel free to ignore.

I would add a proper error type here. It is not too much work, and you have the benefit of having more readable code, because you are not mixing business logic and error messages. Additionally, it is much easier to use miette's advanced features such as error codes that way without cluttering the code itself too much.

Example:

#[derive(thiserror::Error, miette::Diagnostic, Debug)]
pub enum DependencyLocatorError {
    #[error("locator {dependency} is missing a repository delimiter")]
    #[diagnostic(
        code = "locator_missing_dependency",
        help = "try to check your dependencies to ensure they are properly formatted"
    )]
    MissingDelimiter {
        dependency: String,
    },

    #[error("locator {dependency} is missing a repository delimiter")]
    #[diagnostic(
        code = "locator_missing_version",
        help = "make sure you specify the version you want"
    )]
    MissingVersion {
        dependency: String,
    },

    #[error("invalid package name")]
    InvalidPackageName(#[from] #[diagnostic_source] PackageNameError),
}

And then you use this in the code like this:

let (repository, dependency) = dependency
    .trim()
    .split_once('/')
    .ok_or_else(|| DependencyLocatorError::MissingDelimiter { dependency })?;

Only when you tie them all together (for example, in the main function) you should use miette::Report instead.

My feeling is that I would not allow using miette::Report in FromStr implementations. It is perfectly fine to use for the main function, where you are tying together errors from various sources. But in things such as FromStr or TryFrom, I generally don't allow the use of non-structured errors.

Originally posted by @xfbs in #131 (comment)

@mara-schulke mara-schulke self-assigned this Oct 26, 2023
@mara-schulke mara-schulke added docs::crate Improvements or additions to documentation complexity::low Issues or ideas with a low implementation cost component::cli Everything related to the buffrs cli priority::high Urgent change or idea, please review quickly type::refactoring Changing the inner-workings of buffrs labels Oct 26, 2023
@mara-schulke mara-schulke added this to the User Experience milestone Oct 26, 2023
@mara-schulke mara-schulke moved this to In Progress in Buffrs Oct 26, 2023
@mara-schulke mara-schulke moved this from In Progress to Todo in Buffrs Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity::low Issues or ideas with a low implementation cost component::cli Everything related to the buffrs cli docs::crate Improvements or additions to documentation priority::high Urgent change or idea, please review quickly type::refactoring Changing the inner-workings of buffrs
Projects
Status: Todo
Development

No branches or pull requests

1 participant