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

Improve parsing error type #559

Closed
QnnOkabayashi opened this issue Nov 11, 2021 · 1 comment
Closed

Improve parsing error type #559

QnnOkabayashi opened this issue Nov 11, 2021 · 1 comment

Comments

@QnnOkabayashi
Copy link
Contributor

QnnOkabayashi commented Nov 11, 2021

Motivation
The current error type, ErrorKind, contains a ton of irrelevant information. For example, the InvalidCharacter variant contains the expected characters, which shouldn't have to be a field carried around. All the fields holding an ExpectedLength are also redundant, because they are known and do not vary.

Solution
Revise the error type to contain more relevant information like the following:

enum UuidParseError {
    /// Non-hex character, potentially a multibyte character
    Char {
        character: char,
        index: usize,
        width: usize,
    },
    /// A simple UUID didn't contain 32 characters
    SimpleLength {
        len: usize,
    },
    /// A hyphenated UUID didn't contain 5 groups
    GroupCount {
        count: usize,
    },
    /// A hyphenated UUID had a group that wasn't the right length
    GroupLength {
        group: usize,
        len: usize,
        offset: usize,
    },
}

Alternatives
We could just not change it...

Is it blocking?
The information contained in the new error type is important for allowing the uuid!() proc-macro provide the most useful feedback, and I don't want to have multiple error types.

I'm pretty sure that this doesn't change the API in any way since the ErrorKind type was always private, so it shouldn't be breaking to any code unless someone was parsing the error message...

@KodrAus
Copy link
Member

KodrAus commented Nov 11, 2021

This seems reasonable to me! The shape of the underlying error is all internal so it can be freely tweaked at any time.

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

No branches or pull requests

2 participants