Skip to content

BREAKING CHANGE: update dataset wrapper#14

Open
jeswr wants to merge 2 commits intomainfrom
chore/update-dataset-wrapper
Open

BREAKING CHANGE: update dataset wrapper#14
jeswr wants to merge 2 commits intomainfrom
chore/update-dataset-wrapper

Conversation

@jeswr
Copy link
Collaborator

@jeswr jeswr commented Feb 12, 2026

  • BREAKING CHANGE: remove match methods from DatasetCoreBase
  • Refactor subjectsOf and objectsOf methods to use toTerm for improved clarity

Copilot AI review requested due to automatic review settings February 12, 2026 14:51
@jeswr jeswr changed the title chore/update dataset wrapper BREAKING CHANGE: update dataset wrapper Feb 12, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the dataset wrapper API by removing the older matchSubjectsOf/matchObjectsOf helpers and refactoring subjectsOf/objectsOf to build RDF/JS Terms via a shared toTerm helper, plus updating docs/tests to the new argument order.

Changes:

  • Remove matchSubjectsOf/matchObjectsOf and implement filtering directly in subjectsOf/objectsOf.
  • Refactor term construction through toTerm(...) for predicate/object/graph (and subject for objectsOf).
  • Update usage in unit model wrapper and README examples to match the new subjectsOf signature.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
test/unit/model/ParentDataset.ts Updates subjectsOf(...) call site to the new argument order.
src/DatasetWrapper.ts Removes match helper methods and refactors subjectsOf/objectsOf to use toTerm.
README.md Updates example to the new subjectsOf(TermWrapperCtor, predicate) call style.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI commented Feb 12, 2026

@jeswr I've opened a new pull request, #15, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link

Copilot AI commented Feb 12, 2026

@jeswr I've opened a new pull request, #16, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Collaborator

@langsamu langsamu left a comment

Choose a reason for hiding this comment

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

The current design was intentional: On one hand,

  1. to facilitate the full power of RDF/JS match and on the other
  2. to expose a readable API.

Match

The RDF/JS DatasetCore.match is the primary interaction point with statements. By its polymorphic versatility, it facilitates all possible graph patterns. Namely, all four arguments of match are optional (Term | undefined).
I would like the library to make this pattern available to inheritors of DatasetWrapper.

Readability

The idea behind the terser select/project utility method name signatures is to reflect something akin to these statements:

Give me the subjects of the predicate p wrapped in (projected onto instances of) c.

My best approximation of the above is a line like

return this.subjectsOf(Vocabulary.hasChild, Parent)

The change proposed here is indeed a simplification if we only consider the code implemented to date.

But if you consider plans for the evolution of the API (which are, admittedly, undocumented for now) then I would rather keep things as they are.

@jeswr
Copy link
Collaborator Author

jeswr commented Feb 12, 2026

On the introduction of string | Term option.

This was more a feature addition than anything; in particular to enable matching over Literals and Blank Nodes in the subject/object position (per #12).

Granted, we can keep this as string in the predicate position - the introduction of string | Term there was as a matter of consistency.

Readability

I don't understand - the following are equivalent in the JS world:

return this.subjectsOf(Parent, Vocabulary.hasChild)
return this.subjectsOf(Parent, Vocabulary.hasChild, undefined, undefined)

Do you care about the order of Parent and Vocabulary.hasChild as parameters? I would interpret what I have here as "Give me the subjects -- wrapped in c -- of the predicate p." To me neither order is easier/harder to read.

@langsamu
Copy link
Collaborator

RE string | Term:
I see now. I understand why matching over literals is a good feature. One could imagine asking a dataset for this.match(undefined, namedNode("p"), literal("o")).map(q = > q.subject).
Let me think about that and comment on #12.

RE readability:
Yes, I do care about the order, but not only the order.

I would like two things:

  1. To expose the versatility of match, so callers can request any pattern.
  2. To have the simplest overload that is shortest to write for the majority case.

This is why I'm trying to keep the verbose method in and also to have a terse utility signature in addition to that.

Specifically putting the predicate first was an explicit request for readability from my first reviewer, Matthieu.

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