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

added DatasetCoreFactory #29

Merged
merged 3 commits into from
Jan 22, 2019
Merged

added DatasetCoreFactory #29

merged 3 commits into from
Jan 22, 2019

Conversation

bergos
Copy link
Member

@bergos bergos commented Jan 10, 2019

This PR fixes #20 by adding a DatasetCoreFactory interface.

In DatasetFactory.dataset() we have the quads argument defined like this: optional (Dataset or sequence<Quad>) quads. I replaced it with optional sequence<Quad> quads as the DatasetCore interface implements sequence<Quad>. Maybe we want to add it again to have it more explicit. I also added a short description for the interface. The rest is just an adapted copy of DatasetFactory.

Copy link
Contributor

@blake-regalia blake-regalia left a comment

Choose a reason for hiding this comment

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

Does it really need to extend DataFactory? I don't see why that would be necessary. Everything else about this looks good to me.

@bergos
Copy link
Member Author

bergos commented Jan 16, 2019

You are right. But we should be careful when we discuss features for the Dataset, which might require new Terms or Quads. Then we need somehow the plain DataFactory. But right now it doesn't matter.

Later in the code I would like to pass around only one factory for Term, Quad and Dataset. Can/should we add something in the spec for that topic?

@blake-regalia
Copy link
Contributor

blake-regalia commented Jan 17, 2019

Later in the code I would like to pass around only one factory for Term, Quad and Dataset.

Hmm.. In fact, there are plenty of use cases for instantiating different implementations of a Dataset simultaneously. If some method wants a to create Terms as well as Datasets, then it should expect a factory argument for each. Merging the DatasetFactory interface with DataFactory for the sake of passing one variable around would not be so much a feature as a limitation.

@bergos
Copy link
Member Author

bergos commented Jan 17, 2019

I agree that there can be good reasons to use different Dataset implementation, but a custom factory is just an Object.assign away. Besides passing them around as a single object, optimized Dataset implementations might rely on it's own Term and Quad code.

I don't want to force anybody to use a combined factory, but I would like to do it. So all I want is avoiding name clashes. It's not that interesting for this spec, but maybe for new specs which will be based on the Representation spec and the Dataset spec. Please suggest better options. I agree that extending the interface is a little bit "heavy" for the feature I want.

This PR does not add this as a new feature. I would propose to accept this PR and I create a new issue for that topic.

@blake-regalia
Copy link
Contributor

blake-regalia commented Jan 19, 2019

@bergos I'm not sure I'm following the parts about new specs and naming clashes but it sounds like you've got some good ideas in mind, maybe I am just not getting it?

Please suggest better options.

I think the best option is that implementations can choose to implement both interfaces simultaneously. This was one of Java's greatest innovations IMO. Please let me know if there is something else I am missing though.

This PR does not add this as a new feature. I would propose to accept this PR

Again, not sure I am getting this right, but isn't this feature exactly what the PR is adding? I think this is the appropriate place then to make a case against extending DataFactory.

@bergos
Copy link
Member Author

bergos commented Jan 21, 2019

@blake-regalia I removed extending from DataFactory. The origin for this was the DatasetFactory, as DatasetCoreFactory is just a light copy of the full featured factory. I created issue #37 for the small concern I have about name clashes.

@bergos bergos merged commit f2945ed into master Jan 22, 2019
@vhf vhf deleted the core-factory branch February 7, 2019 09:55
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.

Add DatasetCoreFactory
4 participants