-
Notifications
You must be signed in to change notification settings - Fork 13
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
Plain, Star, Roles, Patterns and Generics #23
Conversation
|
Would you revert the |
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 me start by saying that I find this PR too big and touching too many areas and without expressing the exact pain points which necessitate bringing in those changes. It also caused a slightly messy diff
- The "Role" concept
- The arguments of dataset, stream and store (could even be separated?)
toArray
Re 3. It's simple enough; I propose it was a separate PR. A breaking change, though?
Re 2. I don't understand fully. I added some comments which address my concerns
I like the idea of "roles" although I would propose a slightly different execution. Please see my branch extract-roles. I pushed (and renamed) out the StarRole
and PlainRole
to separate modules and used them as generics to provide types for quad's components
Thus, Quad
becomes the default "RDF* Quad" and no deprecation is needed.
import {RdfStar} from './data-model/rdf-star'
import {Rdf11} from './data-model/rdf1.1';
interface Quad<R extends Role = RdfStar> extends BaseQuad {
subject: R['Subject'];
predicate: R['Predicate'];
object: R['Object'];
graph: R['Graph'];
}
And yes, I don't understand what the *Pattern
types are for (other than union in match
and similar functions)
import { Stream } from './stream'; | ||
|
||
export interface DatasetCore<OutQuad extends BaseQuad = Quad, InQuad extends BaseQuad = OutQuad> { | ||
|
||
export interface DatasetCore<OutQuad extends BaseQuad = StarQuad, InQuad extends BaseQuad = StarQuad> { |
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.
I do not agree with this change. I remember having discussed this with @bergos. While I agree with you goal to have the "in" type be more accepting so that any Term can be passed as argument, this does not reflect reality.
Implementations may or may not require their specific types being used. It is safer to default to a more strict pattern where same type is enforced. This way the author of library or type declaration has the choice to relax the default. It is as simple as
-export class MyDataset extends RDFJS.DatasetCore<MySpecialQuad> {
+export class MyDataset extends RDFJS.DatasetCore<MySpecialQuad, StarQuad> {
// ...
}
The alternative is that consumers will use MyDataset
unbeknownst that it will break in runtime when anything other that MySpecialQuad
is provided.
@@ -192,13 +185,13 @@ export interface Dataset<OutQuad extends BaseQuad = Quad, InQuad extends BaseQua | |||
*/ | |||
union(quads: Dataset<InQuad>): Dataset<OutQuad, InQuad>; | |||
|
|||
match(subject?: Term | null, predicate?: Term | null, object?: Term | null, graph?: Term | null): Dataset<OutQuad, InQuad>; | |||
match(subject?: InQuad['subject'] | TermPattern, predicate?: InQuad['predicate'] | TermPattern, object?: InQuad['object'] | TermPattern, graph?: InQuad['graph'] | TermPattern): Dataset<OutQuad, InQuad>; |
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.
This change, excuse the pun, does not match the spec.
Regardless, narrowing the parameters to a subset of Term will be a breaking change whenever the inputs were not strictly typed to the more specific type
let term: Term
// this works now
// will fail when merged
dataset.match(term)
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.
I think it will also make TermPattern
redundant?
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.
This change allows the implementor to narrow the subtype of Quad they will accept, for example in a dataset that does not support RDF-Star. I don't see what makes this incompatible with the spec. Isn't this related to what you said above?
Implementations may or may not require their specific types being used.
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.
Right, I see how it can appear contradictory.
While the vectors of the changes are opposite, in both cases I think they risk breaking the current behaviour. As mentioned above, a Dataset
accepts any term in the match
and other methods. Making it more strict will inevitable break the type checks in many projects.
I thought that maybe your changes could work if instead the defaults for type arguments were
-OutQuad extends BaseQuad = Quad
+OutQuad extends BaseQuad = BaseQuad
Thus, defaulting to Term
for all parameters. However, consider type DatasetExt = DatasetCore<QuadExt>
. Same thing happens, that TypeScript will refuse to transpile when Term
is used as argument but rdf-ext
will happily ignore "wrong" terms...
Yes, I'm happy to break these out into smaller, more manageable PRs. It's just that several of these changes are intra-dependent so it made it hard to split some of them up. I understand the veto to |
I would totally separate " We may work out the rest. If you look at my branch I linked to, it appears that some of those changes could be avoided? |
*/ | ||
export type Quad_Object = NamedNode | Literal | BlankNode | Quad | Variable; | ||
export namespace PlainPatern { |
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.
export namespace PlainPatern { | |
export namespace PlainPattern { |
*/ | ||
export type Quad_Subject = NamedNode | BlankNode | Quad | Variable; | ||
export type TermPattern = Variable | null; |
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.
Also undefined
?
Closing in favor of new and separate PRs per conversation. |
This PR addresses several shortcomings of the current typings, introduces a set of narrower 'Plain' types for developers dealing with pre-RDF-star formats such as Turtle 1.1, and includes several fixes for generics, defaults, arguments, and return types. Closes #5
1. Introduction of 'Plain' and 'Star' namespaces
Not all libraries are ready to adopt the RDF-star specification draft and some will be interested in exclusively dealing with RDF 1.1 data. To accommodate the narrower scope of those 'Plain' Term types, this introduces the following namespaces:
1a.
PlainRole
andStarRole
Unions of Term types for the various roles they play in 'plain' RDF 1.1 Data, and RDF-star data, respectively. Each namespace exports the following named types:
Subject
,Predicate
,Object
,Graph
,Datatype
, andQuad
Note that
PlainRole
is an export-only type and not used by@rdfs/types
internally. It is meant to provide developers with a type for working with serialization formats and datasets that do not (yet) support RDF-star, such as Turtle 1.1 .types/data-model.d.ts
Lines 137 to 147 in 0824ab6
types/data-model.d.ts
Lines 162 to 172 in 0824ab6
1b.
PlainPattern
andStarPattern
For each type in
PlainRole
andStarRole
, these pattern interfaces add a union withVariable | null
for the various RDFJS methods that accept term patterns as input.types/data-model.d.ts
Lines 149 to 159 in 0824ab6
types/data-model.d.ts
Lines 174 to 184 in 0824ab6
2. Addition of
PlainQuad
andStarQuad
Without removing
BaseQuad
, these two new interfaces extendBaseQuad
by specifying a stricter term type for each quad component than the formerQuad_{Subject|Predicate|Object|Graph}
interfaces. Most notably,PlainQuad
andStarQuad
components do not have unions withVariable
(those cases are covered separately byPlainPattern
andStarPattern
).Note: Each of the
Quad_{Subject|Predicate|Object|Graph}
interfaces are still made available in the exports and carry the same exact types as before. A deprecation annotation has been added to notify the user to consider using one of the more fitting types.types/data-model.d.ts
Lines 300 to 324 in 0824ab6
types/data-model.d.ts
Lines 274 to 298 in 0824ab6
3.
OutQuad
now defaults toStarQuad
instead ofBaseQuad
The specification says that each component of the 'Quad' interface be a
Term
, hence whyBaseQuad
exists in the typings. In this PR,OutQuad
still extendsBaseQuad
, meaning that developers may still use quad implementations that have unorthodox term types such asLiteral
in the subject position. However, this PR makes it so that if the generics arguments are omitted, thenOutQuad
will default to a quad with the traditional term types (including the RDF-star subject and object types) for each component. This change provides developers with stronger type information by default without removing features nor deviating from the spec.types/data-model.d.ts
Line 330 in 0824ab6
types/dataset.d.ts
Line 8 in 0824ab6
types/dataset.d.ts
Line 51 in 0824ab6
types/dataset.d.ts
Line 58 in 0824ab6
types/dataset.d.ts
Line 199 in 0824ab6
4.
InQuad
now defaults toStarQuad
instead ofOutQuad
InQuad
should not default toOutQuad
if the 2nd generic argument is omitted. For example:Would imply now that the
quad
parameter is typed toMySpecialQuad
foradd(quad)
,delete(quad)
,has(quad)
, etc. -- but the library should accept all RDFJS terms as arguments in order to implement the spec, not justMySpecialQuad
.By defaulting to
InQuad
toStarQuad
, if this argument is omitted, then the types for quad arguments are assumed to be traditional, RDF-star compatible quads.See 3.1, 3.2, 3.3, 3.4, and 3.5 above for code references.
5.
subject
,predicate
,object
andgraph
arguments are now typed toInQuad['{subject|predicate|object|graph}']
Before this change, each argument in match methods (e.g.,
DatasetCore#match()
,Dataset#deleteMatches()
, and so on...) were typed toTerm
. Now these methods are typed by component types within the given genericInQuad
argument, and each one is union'ed with| Variable | null
.types/dataset.d.ts
Line 46 in 0824ab6
types/dataset.d.ts
Line 86 in 0824ab6
types/dataset.d.ts
Line 196 in 0824ab6
types/stream.d.ts
Line 48 in 0824ab6
types/stream.d.ts
Line 104 in 0824ab6
6. Distinguish between
OutQuad
vs.InQuad
generic arguments forSource
andStore
Prior to this PR, the argument types and return type of
Source#match()
(and, by inheritance,Store#match
) were the same; however the implementors should be allowed to accept RDFJS term types as arguments and return a stream of their own implementation ofQuad
. This change allows implementors to specify those two types (OutQuad
andInQuad
) much the same way it is done indataset.d.ts
.Additionally,
Store#removeMatches()
andStore#deleteGraph()
now specifyInQuad
rather than the ambiguousQ
.types/stream.d.ts
Line 38 in 0824ab6
types/stream.d.ts
Line 80 in 0824ab6
types/stream.d.ts
Line 90 in 0824ab6
types/stream.d.ts
Line 116 in 0824ab6
7. Remove
toArray()
Per rdfjs/dataset-spec#64 .