pchampin / sophia_rs

Sophia: a Rust toolkit for RDF and Linked Data
Other
214 stars 23 forks source link

Move from `coercible-error` to a dedicated `StreamError` #28

Closed pchampin closed 4 years ago

pchampin commented 4 years ago

This issue was raised in the discussion about #8 . The argument was that

I'll copy the relevant parts of the discussion below, for the sake of clarity.

pchampin commented 4 years ago

I suggest we introduce (in the triple::stream module) the following type (with the appropriate thiserror annotations)

enum StreamError<E1, E2> {
  UpStream{ source: E1 },
  DownStream{ source: E2 },
}

Seems to me that this type has the best of both worlds from coercible-error and anyhow:

Granted, the return type of stream methods (such as in_sink and in_graph) will be slightly more verbose than with anyhow, but at least it is (I think) idiomatic and easily understandable.

_Originally posted by @pchampin in https://github.com/pchampin/sophia_rs/issues/8#issuecomment-565848667_

pchampin commented 4 years ago

Great! The StreamError is a brilliant idea. I also agree with the 'one module at a time' strategy.

I would only name StreamError's variants a bit different.

enum StreamError<SErr, TErr> {
    Source { source: SErr },
    Target { source: TErr },
}

This way it is more obvious if a error was raised by the TripleSource or the consumer of it.

Another candidate that came into my mind is the either-crate. It is basically a sum type like the suggested StreamError but is not implemented specific as an error type. However, it provides some nice utility functions. I think it will serve as a good template when implementing StreamError.

When I have some time I would write a PR if it's okay for you.

_Originally posted by @MattesWhite in https://github.com/pchampin/sophia_rs/issues/8#issuecomment-565934935_

pchampin commented 4 years ago

Terminology

If we want to be consistent, the variants should be Source/Sink rather than Source/Target.

I find UpStream/DownStream explicit enough, and a little more general (at some point, we might introduce other parts of a stream, such as a filter)... But this might be over-anticipating ;-) So I'm fine with your proposal as well (modulo the adjustment above).

The either crate

The main drawback I see in the either crate is that it does not implement std::error::Error. And I'm not sure how much all the other methods it provides are useful in our case.

I would rather stick to our own type, and add some utility methods to it when we need them...