Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for RDF* #24

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Conversation

rubensworks
Copy link
Contributor

This brings this package up to date with the latest RDF/JS data model spec.

As suggested by @tpluscode, I've made the dependency range of @types/rdf-js as * to avoid future versioning issues. This way we're in line with how DefinitelyTyped handles typings dependencies as well.

\cc @bergos

Copy link

@tpluscode tpluscode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the data factory also needs to be updated. In an esm fork I experimented with RDF* and I figured that dataFactory#quad should ensure that the subject is upgraded to a compliant termType='Quad' structure

Right here I check whether the subject is a quad but without termType:
rdf-esm/data-model@master...rdf-esm:rdf-star#diff-c62ad95856c3c32496eea9c7854ad775R39-R44

And tests are like this:
rdf-esm/data-model@master...rdf-esm:rdf-star#diff-f33e770296d5ce90cf32852e9e73005eR33-R73

@rubensworks
Copy link
Contributor Author

@tpluscode I like the idea, but wouldn't some kind of dedicated ConvertingDataFactory be better for ensuring this?
For factories where we're certain that quad terms are compliant, this just adds unneeded CPU cycles.

Side-note: quad terms can also occur in the object position. (in generalized RDF even in every position)

@tpluscode
Copy link

I'm pretty sure that the RDF* factory should ensure it produces compliant terms. Otherwise you might have unexpected results?

Is there a spec for this? I tried to config what you said but all examples I found for RDF* only used quad term in subject position 🤔

@rubensworks
Copy link
Contributor Author

rubensworks commented Aug 27, 2020

I'm pretty sure that the RDF* factory should ensure it produces compliant terms. Otherwise you might have unexpected results?

Sure, but strictly speaking it should also take in compliant terms :-)

Is there a spec for this? I tried to config what you said but all examples I found for RDF* only used quad term in subject position 🤔

Not yet AFAIK. But here's the formal definition: http://olafhartig.de/files/Hartig_AMW2017_RDFStar.pdf (section 2.1)

@tpluscode
Copy link

Sure, but strictly speaking it should also take in compliant terms :-)

Disagreed. For the sake of interoperability a factory should (must?) accept non-compliant terms. This is why rdf-ext works flawlessly with terms produced by other factories.

Hartig_AMW2017_RDFStar.pdf (section 2.1)

Thanks, that section indeed says that:

RDF⋆ extends the notion of such triples by allowing for triples that have another triple in its subject or its object position

Thus, a similar logic I implemented for subjects should be repeated for objects. This is necessary for robustness as stated by Postel's Law

@rubensworks
Copy link
Contributor Author

rubensworks commented Aug 28, 2020

Let's agree to disagree. I understand your motivation for adding this into the factory, but at the same time I need a data factory that is as lightweight as possible for performance reasons.
If this is not the goal of this package here, feel free to close my PR.

@bergos
Copy link
Contributor

bergos commented Aug 28, 2020

Thanks for the PR. Looks good to me.

I'm pretty sure that the RDF* factory should ensure it produces compliant terms. Otherwise you might have unexpected results?

The spec says We don't define any validation. One can argue if this is already a validation, but the intention was to define only the bare minimum for people that like to have maximum performance or do some kind of validation in another step. Other implementations or implementations on top of this package can still add some kind of validation.

Side-note: quad terms can also occur in the object position. (in generalized RDF even in every position)

I don't think we should focus too much on RDF*. We defined a way to use the Quad as a Term. If some library implements it with RDF* validation, that's fine. If somebody likes to use it differently for a good reason, that's also fine. That's why I also think we should not declare it experimental, just because the RDF* spec is not yet finished. If required we can discuss this more in detail in the RDF* explanation and link issue. Btw. I would be happy about any more detailed proposals for the content for that RDF* explanation for the spec.

@bergos bergos self-requested a review August 28, 2020 23:43
@@ -29,7 +29,7 @@
},
"homepage": "https://github.com/rdfjs-base/data-model",
"dependencies": {
"@types/rdf-js": "^2.0.1"
"@types/rdf-js": "*"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why this dependency is relaxed to * instead of ^4?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A ^4 range excludes future major updates when that package becomes 5.0. This can cause npm to install duplicate versions of the typings package leading to compile issues from TypeScript, especially in monorepo scenarios

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused: compile errors is exactly what one would want if the typings are incompatible, no?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's false negatives I'm talking about. Not really about incompatible types. The error looks similar to

src/index.ts(3,14): error TS2742: The inferred type of 'NamedNode' cannot be named without a reference to '../packages/in-monorepo/node_modules/rdf-js'. This is likely not portable. A type annotation is necessary.

@bergos bergos merged commit ca30c53 into rdfjs-base:master Sep 2, 2020
@bergos
Copy link
Contributor

bergos commented Sep 2, 2020

published as new minor version 1.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants