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

Replace coercible errors #29

Merged
merged 9 commits into from
Dec 20, 2019

Conversation

MattesWhite
Copy link
Contributor

This PR replaces the coercible-errors-crate by StreamError, defined like following:

#[derive(Debug)]
pub enum StreamError<SourceErr, SinkErr>
where
    SourceErr: 'static + Error,
    SinkErr: 'static + Error,
{
    Source(SourceErr),
    Sink(SinkErr),
}

It allows function to return two different types of error, usually either an error from the Source providing new data or from the Sink that consumes it.

to do

As showed are the trait bounds for SourceErr and SinkErr only 'static + Error. This should be expanded by + Send + Sync in the future, in order to be able to implement concurrent graphs. The bound extension is delayed until sophia::error::Error is completly removed.

This PR is the result of #8 and was implemented according to #28

This new error type is designed to replace the `coercible-errors`-crate.
According to pchampin#8 `coercible-errors` is not suitable for the future
seperation of sophia. Therefore, it was replaced by `StreamError` where
sensible.

In addition, `coercoble_errors::Never` was replaced by
`std::convert::Infallible`. It is a first step towards the stabilization of
Rust's `Never`-type.

Resolves: pchampin#28
@MattesWhite
Copy link
Contributor Author

Okay I have no idea why Travis failed. The log seems like it has built an old version of the parser-module? Did I something wrong at commiting?

@pchampin
Copy link
Owner

I'm assuming you didn't use the --all-features option when you ran the tests locally. So you are missing the feature-gated parts of the code (which, it seems, still refer to coercible-error).

I'm about to make a reviw of your PR, so you might want to wait for my comments before putting further work in iit. All in all, this is good, but there are a few changes that I'd like to suggest.

Copy link
Owner

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

That's a good start (and a lot of edits in a lot a files, thanks for that!).

I suggest a few changes to make StreamError implementation lighter and (I think) easier to use and understand for newcomers.

Note that I used github's "suggestion mode" in some comments, but the result would probably not compile as is, so I recommend you do not accept those suggestions directly.

NB: as you may have noticed, I pushed some commits that altered the NT and NQ parsers, but I'll take care of merging the conflict when this PR is good to go.

/// incompatible with `sophia`'s whole error-handling. This should be resolved
/// when the error-handling is completly refactored
/// ([tracking issue](https://github.com/pchampin/sophia_rs/issues/8)).
#[derive(Debug)]
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use thiserror already? This is the plan eventually, and this would reduce the number of lines of code...

Comment on lines 26 to 27
Source(SourceErr),
Sink(SinkErr),
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 we should rename the variants SourceError and SinkError. This is a bit redundant with the name of the enum, but that makes them suitable for public export (so anyone, even outside this crate, can use SourceError rather than the more verbose StreamError::Source).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a pretty good idea. I'll go with that.

Sink(SinkErr),
}

use self::StreamError::*;
Copy link
Owner

Choose a reason for hiding this comment

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

This would then become pub use ...

Comment on lines 37 to 44
/// Build a `StreamError` from an error of the `Source`.
pub fn from_source_err(se: SourceErr) -> Self {
Source(se)
}
/// Build a `StreamError` from an error of the `Sink`.
pub fn from_sink_err(se: SinkErr) -> Self {
Sink(se)
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm really not sure we need this if we publicly expose SourceError and SinkError...
SourceError(err) is much easier than StreamError::from_source_err(err)

Sink(se)
}
/// Checks if `StreamError` was raised by the `Source`.
pub fn is_source_err(&self) -> bool {
Copy link
Owner

Choose a reason for hiding this comment

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

for the sake of homogeneity, this should probably become is_source_error
(same for is_sink_err below)

.map_err(Into::into)?;
let mut to_remove = to_remove.into_iter().as_quad_source();
self.remove_all(&mut to_remove)?;
self.remove_all(&mut to_remove)
.map_err(|err| err.into_sink_err())?;
Copy link
Owner

Choose a reason for hiding this comment

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

according to my suggestion on StreamError, this would become

Suggested change
.map_err(|err| err.into_sink_err())?;
.map_err(|err| err.unwrap_into())?;

I'm guessing the correct type will be inferred.

Ok(self.remove_all(&mut to_remove)?)
Ok(self
.remove_all(&mut to_remove)
.map_err(|err| err.into_sink_err())?)
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above

Suggested change
.map_err(|err| err.into_sink_err())?)
.map_err(|err| err.unwrap_into())?)

.map_err(Into::into)?;
let mut to_remove = to_remove.into_iter().as_triple_source();
self.remove_all(&mut to_remove)?;
self.remove_all(&mut to_remove)
.map_err(|err| err.into_sink_err())?;
Copy link
Owner

Choose a reason for hiding this comment

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

same as above

Suggested change
.map_err(|err| err.into_sink_err())?;
.map_err(|err| err.unwrap_into())?;

@@ -263,7 +264,7 @@ mod test {
let mut d = HashSetDataset::new();
let reader = io::Cursor::new(GENERALIZED_DOC);
let res = STRICT.parse_read(reader).in_dataset(&mut d);
if let Err(Error(ParserError(_, location), _)) = res {
if let Err(StreamError::Source(Error(ParserError(_, location), _))) = res {
Copy link
Owner

Choose a reason for hiding this comment

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

Could be simplied as

Suggested change
if let Err(StreamError::Source(Error(ParserError(_, location), _))) = res {
if let Err(SourceError(Error(ParserError(_, location), _))) = res {

if you accept my suggestions on StreamError

for tr in self {
let t = tr?;
sink.feed(&t)?;
let t = tr.map_source_err()?;
Copy link
Owner

Choose a reason for hiding this comment

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

As suggested in StreamError, this is uselessly unidiomatic.
I would rather write

Suggested change
let t = tr.map_source_err()?;
let t = tr.map_err(SourceError)?;

and do the same with SinkError below

Im favor for a smaller API the variants of StreamError are now public
available.
In addition, various conversion functions where cut in favor of
`StreamError::into_inner()`.
Just did a straight adoption to the new stream-api. Now it should be
possible to use rio's own error type as SourceError.
@pchampin
Copy link
Owner

@MattesWhite is this ready for merge? (guessing so, but just checking)

@MattesWhite
Copy link
Contributor Author

Yes its ready 👍

@pchampin
Copy link
Owner

Ok, I'll try to do it during the week-end. And I'll also try to push and merge my work on #24. The latter is impacting almost all files in the repo, so I advice you don't start any work until it's done -- merging it afterwards would be hell.
Thanks again for your help.

@pchampin pchampin merged commit 79a1b62 into pchampin:master Dec 20, 2019
@MattesWhite MattesWhite deleted the replace-coercible-errors branch January 8, 2020 14:38
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