search

kasei / perlrdf

Deprecated in favor of the Attean package
26 stars 25 forks source link

API inconsistent between RDF::Trine::Store::Memory and RDF::Trine::Store::SPARQL #149

Closed minusdavid closed 7 years ago

minusdavid commented 7 years ago

In RDF::Trine::Store::SPARQL::remove_statements(), it takes a RDF::Trine::Statement as the first argument while RDF::Trine::Store::Memory::remove_statements() takes 4 arguments which are RDF::Trine::Node::Resource objects.

The documentation suggests that both modules use the interface implemented by RDF::Trine::Store::Memory but it's not the case in reality.

Looking at RDF::Trine::Store::DBI, it seems to implement the same API as RDF::Trine::Store::Memory. RDF::Trine::Store::Redis seems to use the same interface as well.

I'd say RDF::Trine::Store::SPARQL is displaying aberrant behaviour.

I want to change it to be consistent with the other store modules, but I know it's a breaking change. Yet, if a person wants to write code that can swap in any storage backend, I think it's essential to implement a consistent API.

I might look at workarounds for the time being, but I'll think about sending in a PR if I can find some time.

minusdavid commented 7 years ago

Then again the workaround would be so much less efficient... so maybe it's worthwhile to get a fix out. @kasei, I'd be really curious what you think about this one. I recall you saying that RDF::Trine::Store::SPARQL wasn't really looked after or used much, so maybe we don't need to worry too much about breaking things? Although I suppose that seems optimistic...

...but not sure how else to regain consistency in the interface :S.

Maybe some syntactical sugar could be added to all the different storage backends... or... just some more careful checking of references. Like if the first argument is RDF::Trine::Statement then treat it like X, if it's RDF::Trine::Node::Resource, treat it like Y.

That might be the safest thing to do?

kasei commented 7 years ago

I'm not overly concerned with breaking existing code that's using an API that is broken (based on documentation). That said, as you say, this does seem like a case where it would be easy to keep the old API working by testing for RDF::Trine::Node arguments. I'd be happy to merge a PR that fixes this.

minusdavid commented 7 years ago

I opted to keep the old API working so as to be backwards compatible as well as supporting the standard function interface as well. There's unit tests to check the SPARQL generated by both, so it seems a reasonable solution.

minusdavid commented 7 years ago

Pull request #150 just submitted.