comunica / sparqlee

⚙️ SPARQL expression evaluator library - Moved to @comunica/expression-evaluator
https://www.npmjs.com/package/sparqlee
14 stars 7 forks source link

Adding the val to the statement of the literal equality check #168

Closed danielbeeke closed 1 year ago

danielbeeke commented 1 year ago

Here is the issue: https://github.com/comunica/sparqlee/issues/167

coveralls commented 1 year ago

Coverage Status

Coverage: 100.0%. Remained the same when pulling 45d7548f17d7fb6aea2782647f5aa79cce4cf988 on danielbeeke:literal-equality-check into 399c68e9c43cdb374fe8ab9f38ec93add551528e on comunica:master.

jitsedesmet commented 1 year ago

This looks good to me! Would it be possible to add a test, so we don't run into this issue again?

danielbeeke commented 1 year ago

Yes, Ill create a test

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

danielbeeke commented 1 year ago

Ah I made a mistake with the commit. I used a wrong username. I have now signed it with both GitHub accounts.

rubensworks commented 1 year ago

Summarization: It might be weird that an error is thrown when comparing 2 non-equal languageStrings and not when comparing 2 non-equal strings, but this should be the behavior according to the spec. I tried to list the reason in a comment here: https://github.com/comunica/sparqlee/pull/168#discussion_r1120072061

It's a bit weird indeed, however correct according to the spec.

Since we all agree it's weird, I suggest that we go beyond the spec, and properly support the comparison on rdf:langString's as well, which should require minimal effort AFAICS. Since SPARQL allows operator extensibility for undefined operations, this will still be spec-compliant.

jitsedesmet commented 1 year ago

I agree. And this should go fluently, by using the set command after setting the stringy overload on the equality. Between these lines: https://github.com/comunica/sparqlee/blob/master/lib/functions/RegularFunctions.ts#L96-L97

danielbeeke commented 1 year ago

I am not sure if I understand everything, but I gave it a try

@jitsedesmet could you have a look if this is in the right direction?

rubensworks commented 1 year ago

Released as 2.2.1.

danielbeeke commented 1 year ago

Thank you @jitsedesmet and @rubensworks for persisting with me and reviewing!