search

linkeddata / rdflib.js

Linked Data API for JavaScript
http://linkeddata.github.io/rdflib.js/doc/
Other
565 stars 143 forks source link

Improve typing of DataFactory #406

Closed RubenVerborgh closed 4 years ago

RubenVerborgh commented 4 years ago

Sorry, I don't fully understand:

Works mostly ok with maslib - there's one bug though, which is that it requires termToNQ in canonical-data-factory.ts to handle terms that are graphs.

Do you have a code example of what breaks?

megoth commented 4 years ago

Sorry, I don't fully understand:

Works mostly ok with maslib - there's one bug though, which is that it requires termToNQ in canonical-data-factory.ts to handle terms that are graphs.

* Is the bug that `termToNQ` requires handling graphs (as in: it should not)?

* Or is the bug that `termToNQ` does not handle graphs?

Do you have a code example of what breaks?

Sorry for my imprecisessness - if I build the data browser with the current changes, I get the following error:

Error: Can't serialize nonstandard term type (was 'Graph')
  https://megoth.localhost:8443/mashlib.js:7308:15
    termToNQ  https://megoth.localhost:8443/mashlib.js:7230:23
    id  https://megoth.localhost:8443/mashlib.js:7432:42
    id  https://megoth.localhost:8443/mashlib.js:10100:30
    id  https://megoth.localhost:8443/mashlib.js:19608:38
    canon  https://megoth.localhost:8443/mashlib.js:20227:27
    statementsMatching  https://megoth.localhost:8443/mashlib.js:10197:22
    each  https://megoth.localhost:8443/mashlib.js:67316:24
    outlineObjectTD  https://megoth.localhost:8443/mashlib.js:4114:35
    refresh/<  https://megoth.localhost:8443/mashlib.js:90436:32
    syncTableToArray  https://megoth.localhost:8443/mashlib.js:4107:18
    refresh  https://megoth.localhost:8443/mashlib.js:4121:7
    render  https://megoth.localhost:8443/mashlib.js:67943:56
    renderPane  https://megoth.localhost:8443/mashlib.js:67975:47
    listen/<  undefined

I've pushed a possible solution to feature/extended-typing-toNQ-fix, but not sure if this is correct.

RubenVerborgh commented 4 years ago

Eh where does Graph even come from? It is most definitely not an RDF type, nor RDF/JS type. It shouldn't exist. Let me check that.

RubenVerborgh commented 4 years ago

Ok seems like we're still doing

export const CollectionTermType = "Collection" as const
export const EmptyTermType = "Empty" as const
export const GraphTermType = "Graph" as const

stuff, which we really should not.

However, my main mission was to unblock you on #397, which I think I've achieved.

I think the route for fixing the problem is getting rid of GraphTermType, as there really is no such thing; they are either named, blank, or default. In the meantime, workaround extended-typing-toNQ-fix will do.

megoth commented 4 years ago

Ok seems like we're still doing

export const CollectionTermType = "Collection" as const
export const EmptyTermType = "Empty" as const
export const GraphTermType = "Graph" as const

stuff, which we really should not.

Should we create a ticket to track this?

However, my main mission was to unblock you on #397, which I think I've achieved.

Yes, thank you again 😸

I think the route for fixing the problem is getting rid of GraphTermType, as there really is no such thing; they are either named, blank, or default. In the meantime, workaround extended-typing-toNQ-fix will do.

Yeah, I think we should create a ticket to track the issue, and then remove the hack introduced in feature/extended-typing-toNQ-fix.

I've created https://github.com/linkeddata/rdflib.js/pull/407 that can be merged when everything's in order.

pmcb55 commented 4 years ago

Just my 2cents, but perhaps this strange looking 'Graph' type is one of the N3 types that Tim mentioned were extensions to the standard RDF types...!?

RubenVerborgh commented 4 years ago

It's a misnomer in any case then. Notation3 has formulas.

timbl commented 4 years ago

It's a misnomer in any case then. Notation3 has formulas.

N3 has things it calls formulas and other people call graphs. So not really a misnomer. (Like Symbol/NamedNode, Formula/Graph, Statement/Arc, etc these are differences between talking about it as a logic language and as a graph. It is useful to be able to look at it either way, as for core RDF a lot of people thing of the graph, but for N3 you have a language which is clean superset of the RDF but where is more than a graph)

I suspect the solution is to check if the given thing is of termtype Graph, and if so raise a run time value error, saying ' This is N3 data, it is too complex to be serialized as an NQuad item" or words to that effect. I always thought we would need run-time errors when we build the system which mix N3 and and base RDF. It is fine to have people use rdflib.hs most of the time for plain RDF, but it would be be bad for the types they are using not t allow them to be able to express more complex things wher ethey ned to. JUst f they try to serialize a query or a rule etc in say turtle ot RDF/XML the system will raise a run time error not a compile time type mismatch.

RubenVerborgh commented 4 years ago

I suspect the solution is to check if the given thing is of termtype Graph, and if so raise a run time value error, saying ' This is N3 data, it is too complex to be serialized as an NQuad item" or words to that effect.

I think a specific termtype for Graph neither necessary nor desired.

Graphs are identified by URIs (or blank nodes), so I'm missing arguments as to why they would be a separate thing.

As a note, N3.js parses Notation3 and remains within the RDF/JS model.

elf-pavlik commented 4 years ago

I've noticed #404, maybe it would be possible to have rdflib.js stick to RDF and have distinct n3.js depend on it and extend it with N3 specific functionality. I can imagine developers intending to use rdflib.js to handle RDF getting confused if they see some N3 errors thrown at them.

megoth commented 4 years ago

Seems that we need to fix one more type error that was hidden with @ts-ignore, namely an incorrect extension of Literal: https://github.com/linkeddata/rdflib.js/blob/master/src/literal.ts#L16

Will try to fix this tonight to get this merged and a new version of rdflib released.

megoth commented 4 years ago

Will try to fix this tonight to get this merged and a new version of rdflib released.

An ugly fix possible, pushed as https://github.com/linkeddata/rdflib.js/pull/414. If this is ok, I think I should be able to merge this to master and release a new version of rdflib.