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

Better Errors #29

Merged
merged 4 commits into from
Aug 3, 2017
Merged

Better Errors #29

merged 4 commits into from
Aug 3, 2017

Conversation

torkleyy
Copy link
Contributor

@torkleyy torkleyy commented Aug 2, 2017

Fixes #21

Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is extremely helpful and needed, thank you!
Would you be able to add a few parser error tests as well as address my notes? I'm fine with follow-ups too :)

src/de.rs Outdated

use serde::de::{self, Deserializer as Deserializer_, DeserializeSeed, Visitor};

pub type Result<T> = ::std::result::Result<T, Error>;

#[derive(Clone, Debug, PartialEq)]
pub enum Error {
Message(String),
Parser { kind: ErrorKind, pos: Position },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: doesn't look like names are useful here, could just be Parser(ErrorKind, Position)

src/de.rs Outdated
}

#[derive(Clone, Debug, PartialEq)]
pub enum ErrorKind {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be ParserError or even Parserror?

@torkleyy
Copy link
Contributor Author

torkleyy commented Aug 3, 2017

I have addressed your request, will merge it now because I want to continue work and start another PR. In case you find there are other changes needed, I'll address them in a future PR.

bors r+

bors bot added a commit that referenced this pull request Aug 3, 2017
29: Better Errors r=torkleyy

Fixes #21
@bors
Copy link
Contributor

bors bot commented Aug 3, 2017

Build succeeded

@bors bors bot merged commit ba88f45 into ron-rs:master Aug 3, 2017
@torkleyy torkleyy deleted the error branch August 3, 2017 12:58
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