rdflib.js icon indicating copy to clipboard operation
rdflib.js copied to clipboard

Update applyPatch function and add tests for it

Open Jan-PieterBaert opened this issue 3 years ago • 2 comments

The applyPatch function had a few small bugs when I tried it, so I fixed those and added tests for the function

Jan-PieterBaert avatar Sep 06 '22 11:09 Jan-PieterBaert

Hi and thanks for your contribution. Can you please describe what bugs you experienced and how your changes fix those?

angelo-v avatar Sep 18 '22 14:09 angelo-v

The bugs I found:

  • the substitute function of a collection didn't seem to work with the current constructor, creating a new collection and adding the elements did work
  • in the applyPatch function patch['insert'] was always used, but in the function declaration it said 'patch'
  • the ds = ds.statements line meant that if there was a where clause this statement would be executed twice instead of once which meant there would suddenly need to be a ds.statements.statements
  • since the other functions seem to assume ds is a collection and the elements of a collection are accessed via elements and not statements I edited that as well
  • the targetKB.query function failed because the initBindings was always undefined, the code seemed to assume it's a list so I made it an empty list

Jan-PieterBaert avatar Sep 18 '22 20:09 Jan-PieterBaert

Looking back at this now it is merged (and broken for solid-ui.) ds was designed to be an Array, not a Collection - which code expects it to be a Collection?

timbl avatar Feb 23 '23 09:02 timbl

So the test is wrong too. The parameters are expected to be IndexedFormulas, not collections. Let's roll this back. Sorry I don't read it carefully in review.

timbl avatar Feb 23 '23 10:02 timbl