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

Revised Roles and Patterns #24

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

blake-regalia
Copy link
Contributor

This PR supersedes parts of #23, namely the support of both RDF 1.1 and RDF-star simultaneously. The revised version of this feature no longer introduces any breaking changes.

Instead, the Role and Pattern namespaces expose each term position with an optional generic argument that specifies the mode of RDF to use (and defaults to RDF-star). See code for more details.

If the consensus agrees with this direction, I will follow up with the corresponding test cases as well before suggesting PR for merge.

@changeset-bot
Copy link

changeset-bot bot commented Oct 12, 2021

⚠️ No Changeset found

Latest commit: 327c78e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@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'm not stoked about the string approach for roles.

Have you had a chance to see my proposal for defining and interface for Role and exporting roles as matching interfaces?

feat/roles...tpluscode:extract-roles

@@ -204,33 +272,59 @@ export interface BaseQuad {
/**
* An RDF quad, containing the subject, predicate, object and graph terms.
*/
export interface Quad extends BaseQuad {
export interface Quad<
RdfMode extends AllowedRdfMode=RdfMode_star,
Copy link
Contributor

Choose a reason for hiding this comment

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

By constraining to AllowedRdfMode you essentially close the path for 3rd party modes, contradicting (?) your comment about "easyRDF"

Copy link
Member

Choose a reason for hiding this comment

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

I would also be in favor of a mechanism that enables developers to add other modes.
The approach from @tpluscode seems to enable this, so perhaps a combination of both approaches could be applied?

export type RdfMode_star = 'rdf-star';

/* Modes officially supported/allowed by RDFJS */
export type AllowedRdfMode = RdfMode_11 | RdfMode_star;
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we want to add generalized RDF here as well?
This was the original motivation for adding the BaseQuad.

@@ -204,33 +272,59 @@ export interface BaseQuad {
/**
* An RDF quad, containing the subject, predicate, object and graph terms.
*/
export interface Quad extends BaseQuad {
export interface Quad<
RdfMode extends AllowedRdfMode=RdfMode_star,
Copy link
Member

Choose a reason for hiding this comment

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

I would also be in favor of a mechanism that enables developers to add other modes.
The approach from @tpluscode seems to enable this, so perhaps a combination of both approaches could be applied?

@blake-regalia
Copy link
Contributor Author

@tpluscode @rubensworks The point of using a string for RDF mode is two-fold. First, it is open for extensibility (AllowedRdfModes is simply there to provide a union term for what RDFJS officially 'supports'). In fact, that was the starting motivation. RDF 1.1, RDF-star, easierRDF, the list goes on. We should strive to be forward compatible with new modes and the string approach is akin to using strings for term types in the spec ('NamedNode', 'BlankNode', etc). Secondly, it enables developers to create for more advanced conditional typings. For example, consider this:

/**
 * `<RdfMode extends AllowedRdfMode=RdfMode_11> => TermTypeKey`
 *
 * Returns the union of valid .termType string values for Terms that appear in the datatype position for the given `RdfMode`
 */
type DatatypeTypeKey<
	RdfMode extends AllowedRdfMode=RdfMode_11,
> = Merge<
	{[K in RdfMode_11 | RdfMode_star]: NamedNodeTypeKey},
	{[K in RdfMode_easier]: DataTypeKey}
>[RdfMode];

@tpluscode
Copy link
Contributor

tpluscode commented Oct 15, 2021

extends in type param does not work the way I think you expect it to. See this example in TS playground

import { NamedNode, BlankNode, Quad } from 'rdf-js'

type RdfMode_11 = 'rdf-1.1'
type RdfMode_star = 'rdf-star'
type AllowedRdfMode = RdfMode_11 | RdfMode_star

type Subject<
  RdfMode extends AllowedRdfMode=RdfMode_star,
> = NamedNode | BlankNode | (RdfMode extends RdfMode_star? Quad: never)

type RdfStarSubject = Subject<'rdf-star'>

type EasyRdfSubject = Subject<'EasyRDF'>

You cannot add additional types because the constraint literally allows only 'rdf-1.1' and 'rdf-star'

@blake-regalia
Copy link
Contributor Author

@tpluscode lol, I understand what extends does... see v5 dev here. I think you might have misunderstood the point I am making about extensibility. Forget about AllowedRdfMode for a second.

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.

3 participants