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

Add map functions for Error<> and Info<> ranges. (#86) #87

Merged
merged 4 commits into from
Feb 20, 2017

Conversation

ncalexan
Copy link
Contributor

It's possible to map_err_range for ParseResult<> too, but it's
awkward because the output type needs to be a compatible StreamOnce.
As suggested in
#86 (comment),
it's probably best to either change the parse result type entirely, or
wait for rust-lang/rust#21903.

This at least helps consumers convert ParseError<> into something
that can implement std::fmt::Display.

It's possible to `map_err_range` for `ParseResult<>` too, but it's
awkward because the output type needs to be a compatible `StreamOnce`.
As suggested in
Marwes#86 (comment),
it's probably best to either change the parse result type entirely, or
wait for rust-lang/rust#21903.

This at least helps consumers convert `ParseError<>` into something
that can implement `std::fmt::Display`.
@ncalexan
Copy link
Contributor Author

@Marwes as I say, I'm okay with pushing this into Mentat if you don't feel it's appropriate. I'm not 100% sure that's possible though, since the enums are pub but the branches aren't marked pub.

Owned(s) => Owned(s),
Borrowed(x) => Borrowed(x),
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add a map_token function as well?

@@ -110,6 +122,18 @@ pub enum Error<T, R> {
Other(Box<StdError + Send + Sync>),
}

impl<T, R> Error<T, R> {
pub fn map_err_range<F, S>(self, f: F) -> Error<T, S> where F: FnOnce(R) -> S {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it is enough to call this map_range.

Message(x) => Message(x.map_range(f)),
Other(x) => Other(x),
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

+ map_token

@Marwes
Copy link
Owner

Marwes commented Feb 17, 2017

@Marwes as I say, I'm okay with pushing this into Mentat if you don't feel it's appropriate. I'm not 100% sure that's possible though, since the enums are pub but the branches aren't marked pub.

With branches, do you mean enum variants (Info::Token)? Variants are always public (for pub enum).

@ncalexan
Copy link
Contributor Author

@Marwes I added follow-up to address your review comment, and thought I might as well make it easy for the next poor fool to copy-paste the conversion to StdError. See what you think.

@Marwes
Copy link
Owner

Marwes commented Feb 20, 2017

Failing on the rust 1.11 test on travis. I am not to attached to preserving backwards functionality but prefixing the std::error::Error with :: will do the trick (and is probably the easiest fix anyway).

@Marwes
Copy link
Owner

Marwes commented Feb 20, 2017

Managed to push to this PR so it should be fixed after travis finishes.

@Marwes Marwes merged commit f317fe7 into Marwes:master Feb 20, 2017
@ncalexan
Copy link
Contributor Author

@Marwes could you push a release including this sometime soon? Thanks!

@Marwes
Copy link
Owner

Marwes commented Feb 22, 2017

I will, just want to add some more docs for #85.

@Marwes
Copy link
Owner

Marwes commented Feb 22, 2017

Just released 2.3.0

@ncalexan
Copy link
Contributor Author

Just released 2.3.0

Many thanks, @Marwes!

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