-
Notifications
You must be signed in to change notification settings - Fork 7
Add SPARQL-star operators and comparison #179
Conversation
PR fails as this requires joachimvh/SPARQLAlgebra.js#98 to be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me. I have some minor 'hints' that might make the code more consistent with the rest, but it's honestly just nit-picking.
(<E.BooleanLiteral> op.apply([ (<Quad> left).object, (<Quad> right).object ], context)).coerceEBV(), | ||
); | ||
}, | ||
false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we not handle invalid lexicals here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and in the other functions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm not sure what this flag does exactly. The documentation didn't help me out much.
What would change when setting this to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag (when true = default value) makes sure that the current implementation throws an error when passed an invalid literal. (some functions don't want this, since they handle invalid literals themselves (see equality of terms))
I don't think this implementation will run into trouble so we can keep it as is.
.set( | ||
[ 'quad', 'quad' ], | ||
context => ([ left, right ]) => { | ||
const op: RegularFunction = new RegularFunction(RegularOperator.EQUAL, equality); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we already use the cast made by regularFunctions
in index.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't due that due to the circular dependency.
import type { ISuperTypeProvider, SuperTypeCallback, TypeCache, GeneralSuperTypeDict } from './TypeHandling'; | ||
import { getSuperTypeDict } from './TypeHandling'; | ||
|
||
// Determine the relative numerical order of the two given terms. | ||
/** | ||
* @param enableExtendedXSDTypes System will behave like when this was true. @deprecated | ||
*/ | ||
export function orderTypes(termA: RDF.Term | undefined, termB: RDF.Term | undefined, isAscending: boolean, | ||
export function orderTypes(termA: RDF.Term | undefined, termB: RDF.Term | undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orderTypes
is an exported function, can we just change the behavior like this? (requires major release?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just do a major update then :-)
The isAscending
option felt a bit weird to me, and complicated the implementation too much, so I removed it.
50fbfe4
to
04af322
Compare
Partially built on top of @jeswr's WIP PR from #161.
@jitsedesmet I changed the ordering logic quite a bit (the old version had a fundamental flaw where
>
was considered the opposite to<
), so it might be good for you to check if I didn't break anything major (all unit and spec tests still pass).