pchampin / sophia_rs

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

Alternative API for SPARQL support #91

Closed pchampin closed 3 years ago

pchampin commented 3 years ago

This is an alternative to #90, following @yever's comments.

I think I prefer this one ;-) @Tpt what do you think?

pchampin commented 3 years ago

@yever In a comment above, you wanted to make it easier for developers to handle results whose kind is known in advance.

With the commit just above, it is now possible to write:

    for binding in d.query("SELECT * {...}")?.into_bindings() {
        ...
    }
    // or
    if d.query("ASK {...}")?.into_boolean() {
        ...
    }

Is that enough for you? Or do you still think additional methods (ask_query, select_query) should be added? Adding them would be straighforward, but personally, I tend to prefer a single method, and the pattern above.

lulu-berlin commented 3 years ago

@yever In a comment above, you wanted to make it easier for developers to handle results whose kind is known in advance.

With the commit just above, it is now possible to write:

    for binding in d.query("SELECT * {...}")?.into_bindings() {
        ...
    }
    // or
    if d.query("ASK {...}")?.into_boolean() {
        ...
    }

Is that enough for you? Or do you still think additional methods (ask_query, select_query) should be added? Adding them would be straighforward, but personally, I tend to prefer a single method, and the pattern above.

~Sorry, I overlooked those into_ methods.~ Thanks for adding these methods :) I'm still a bit concerned about panicking... ;)

What I would have liked is to have the query method itself express in its Result type an error such as "This SparqlResult is not a Bindings".

Maybe a reasonable middle ground here is to make the into_ methods return a Result, so that the client code can question-mark both:

for bindings in dataset.query("SELECT ...")?.into_bindings()? {
    ...
}

so that eventually the client function can join these Results and collect all possible errors, including "wrong result type".

It's true that it's mostly the client code's fault if they used into_bindings() and got a different result type, but I think it's a nicer dev experience if this doesn't blow up your program, but rather handles politely the mistake.

pchampin commented 3 years ago

@yever

I'm still a bit concerned about panicking... ;)

I think of those methods as very similar to unwrap, i.e. you should only call them if you are 100% about the pre-condition. In doubt, you should use a match (or if let) instead.

It's true that it's mostly the client code's fault if they used into_bindings() and got a different result type, but I think it's a nicer dev experience if this doesn't blow up your program, but rather handles politely the mistake.

Result adds a little overhead, which is acceptable to handle errors that may be caused by unexpected data or runtime conditions. It should not be used, IMO, to try and catch developer's mistakes -- those must be detected beforehand, and therefore should not need any handling at runtime.

lulu-berlin commented 3 years ago

I was convinced by your last comment. I created a pull request into this pull request with some tests (#92).

I wanted to suggest adding a reference to the SPARQL issue (#19) also to this pull request.

pchampin commented 3 years ago

Sorry for keeping this stalling for so long. I pushed another commit with

I was reluctant to abandon totally the idea that queries could be pre-processed once and reused multiple times... This proposal makes it possible, but does not impose on implementers to do anything about it (String provides a fallback implementation for Query).

@Tpt, this notion of "prepared query" is quite different from the one in Oxigraph, but I think they can cohabit. It seems to me that oxigraph::sparql::Query would be a good candidate for it, but if I am wrong, you can still use String as a fallback.

What do you guys think?

Tpt commented 3 years ago

@Tpt, this notion of "prepared query" is quite different from the one in Oxigraph, but I think they can cohabit. It seems to me that oxigraph::sparql::Query would be a good candidate for it.

Yes, indeed, oxigraph::sparql::Query is a great candidate for it. I was also planning to remove from Oxigraph 0.2 "prepared query" that does not do much outside parsing to only use this oxigraph::sparql::Query type. I guess we are moving in the same direction.

lulu-berlin commented 3 years ago

I like the added docs and the support for prepared queries.