search

comunica / sparqlee

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

Refactor away redundency in inequality functions #181

Closed jitsedesmet closed 1 year ago

jitsedesmet commented 1 year ago

This PR aims to resolve https://github.com/comunica/sparqlee/issues/178 (working together but not dependent on each other is: https://github.com/comunica/sparqlee/pull/180)

coveralls commented 1 year ago

Coverage Status

coverage: 100.0%. remained the same when pulling 8c896a7ed718e21034895e50329077df36dacc8c on jitsedesmet:refactor/remove-ordering-redundency into 6c22f3774dcac333c131710f0a6843d9f25c1d8c on comunica:master.

jitsedesmet commented 1 year ago

@rubensworks given the implementation in https://github.com/comunica/sparqlee/pull/180 is what you want for Ordering, I would keep making the regular functions call the regular functions since it is more self-contained. Also, if we would want to call ordering, we would not want the fall through case as mentioned in a commend I made (and later resolved in favor of this PR)

rubensworks commented 1 year ago

Also, if we would want to call ordering, we would not want the fall through case as mentioned in https://github.com/comunica/sparqlee/pull/180#discussion_r1257265191

Ok, then we can change that I guess? That's the same reason why I added the strict parameter on orderTypes in d1e01c353c9c2ae810cece7ec4ff209ec1a57dd6.

jitsedesmet commented 1 year ago

But why would you want this? I feel like this case is way better? You have all the logic of the regularfunctions in this one file? Why would you want to go: RegularFunction -> Ordering -> RegularFunction when you can do RegularFunction -> RegularFunction?

rubensworks commented 1 year ago

But why would you want this? I feel like this case is way better? You have all the logic of the regularfunctions in this one file? Why would you want to go: RegularFunction -> Ordering -> RegularFunction when you can do RegularFunction -> RegularFunction?

~~Not sure I understand you correctly. What I suggest is to just delegate to the orderTypes function. AFAIK, orderTypes has no external references back to regular functions.~~

I just looked again at #180, and now I understand your comment, because you did add a back-reference to regular functions from type ordering there.

rubensworks commented 1 year ago

Closes comunica/sparqlee#178