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

Make it easier to get a TextPos from an error #7

Closed
cormacrelf opened this issue Dec 2, 2018 · 6 comments
Closed

Make it easier to get a TextPos from an error #7

cormacrelf opened this issue Dec 2, 2018 · 6 comments

Comments

@cormacrelf
Copy link

Hi, love your work on this, I'm using it to parse CSL for a new citation processor and it's fantastic.

CSL is mostly written by hand and involves big enums of allowed attribute values, so it's important to point out when and where these mishaps happen. I use the TextPos embedded in each variant of roxmltree::Error to produce a codespan for each error, and this works well.

However, it's probably better that this big match statement lives alongside its definition so other users can benefit and get the TextPos easily, and without having to update if roxmltree adds new variants. It would probably live on impl Error. Here is my source:

fn get_pos(e: &Error) -> TextPos {
    use xmlparser::Error as XP;
    match *e {
        Error::InvalidXmlPrefixUri(pos) => pos,
        Error::UnexpectedXmlUri(pos) => pos,
        Error::UnexpectedXmlnsUri(pos) => pos,
        Error::InvalidElementNamePrefix(pos) => pos,
        Error::DuplicatedNamespace(ref _name, pos) => pos,
        Error::UnexpectedCloseTag { pos, .. } => pos,
        Error::UnexpectedEntityCloseTag(pos) => pos,
        Error::UnknownEntityReference(ref _name, pos) => pos,
        Error::EntityReferenceLoop(pos) => pos,
        Error::DuplicatedAttribute(ref _name, pos) => pos,
        Error::ParserError(ref err) => match *err {
            XP::InvalidToken(_, pos, _) => pos,
            XP::UnexpectedToken(_, pos) => pos,
            XP::UnknownToken(pos) => pos,
        },
        _ => TextPos::new(1, 1)
    }
}
@RazrFalcon
Copy link
Owner

Thanks.

Maybe it will be better to extract the TextPos from the enum completely. So Error will became:

pub struct Error {
    pub kind: ErrorKind,
    pub pos: TextPos,
}

At the moment, I don't care about the Error itself, because I'm using it only to print the messages.

@cormacrelf
Copy link
Author

I think that works well. If you wanted to give even more error printing control, it might also be good not to include the position in the Display implementation, because library consumers can just do format!("{}, at {}", kind, pos). Eventually, errors in std will also have the ability to track their causes, so the cause could then be removed from the Display impl as well.

@RazrFalcon
Copy link
Owner

I think that this a very rare case. And as long as there is not an official/popular pattern for error handling I think I will stick with the current one.

@cormacrelf
Copy link
Author

Fair enough.

@RazrFalcon
Copy link
Owner

Done. I've ended up using you method, because a variant with the Error struct is too verbose.

@cormacrelf
Copy link
Author

Great! Thank you.

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