rdfjs / stream-spec

RDF/JS: Stream interfaces – A specification of a low level interface definition representing RDF data independent of a serialized format in a JavaScript environment.
https://rdf.js.org/stream-spec/
5 stars 2 forks source link

define Constructor for Source, Sink and Store #10

Closed elf-pavlik closed 5 years ago

elf-pavlik commented 5 years ago

it tries to address #6 and #5 and looks at current implementations in @rdfjs orga

elf-pavlik commented 5 years ago

@jacoscaz this might affect https://github.com/beautifulinteractions/node-quadstore#rdfstore-class which uses dataFactory in options and has it as second argument.

Would you see possibility of passing abstractLevelDOWN in the options object?

RdfStore({
  dataFactory,
  abstractLevelDOWN: memdown()
})

Or maybe using partial application?

RdfStore(memdown())({ dataFactory })
elf-pavlik commented 5 years ago

@rubensworks it also seems to affect https://github.com/rdfjs/rdfxml-streaming-parser.js#configuration

I see it also uses dataFactory which means currently even different repositories in @rdfjs orga don't have compatible constructors.

jacoscaz commented 5 years ago

@elf-pavlik no issues for us! We'll just release a new major version including this and other breaking changes (if any).

elf-pavlik commented 5 years ago

Thanks for review @rubensworks @bergos @blake-regalia @RubenVerborgh I know you all also maintain streaming parsers, I hope you will have chance to review this PR

bergos commented 5 years ago

I used factory in the packages, cause I would use a single factory object, which provides all factory methods from the data-model, dataset and upcoming specs (prefix could be such a candidate), in my code. With prefix and CURIE support, the factory would go more in the direction of an environment, like defined in the RDF Interfaces spec. With that in mind, I'm a little bit in favor for factory. Technical it's not a problem to pass the factory to multiple factory options, so if the majority is for dataFactory, I would also add support for both.

elf-pavlik commented 5 years ago

I used factory in the packages, cause I would use a single factory object, which provides all factory methods from the data-model, dataset and upcoming specs (prefix could be such a candidate), in my code.

I think it may make sense to have factory print out deprecation warring, otherwise developers might get used to non standard factory and get caught by surprise where other constructors don't support it. Even worst if they pass specific factory and implementation of S* will use a different default one instead, since standard dataFactory was undefined.

When we add support for prefixMap I think it should come as additional option parameter, just as baseIRI or whatever we may need for JSON-LD context for serializers which support compacting. I see difference here between DataFactory and RDFEnvironment which seems to include prefixes.

bergos commented 5 years ago

Maybe it would be easier to define a single ConstructorOptions interface, which contains all possible options. For example:

I'm not sure if we can think of use cases for all possible combinations as of the time of writing the spec.

elf-pavlik commented 5 years ago

I'll make that change tomorrow if no one argues against it.

elf-pavlik commented 5 years ago

I have unified all options into single ConstructorOptions following @bergos suggestion.

elf-pavlik commented 5 years ago

I will give this PR 2-3 more days and merge it. My last change should not affect the 2 approvals it already received.