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.

Sign up for GitHub

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

fix: border case for deleteList #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: border case for deleteList #68

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3671e3
fix: border condition for deleteList
cristianvasquez Oct 11, 2022
e3a43ff
feat: new border cases to deleteList
cristianvasquez Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/Context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,12 @@ class Context {
deleteItems (start) {
let quads = [start]

while (!quads[quads.length - 1].object.equals(this.namespace.nil)) {
let moreQuads = quads.length
while (!quads[quads.length - 1].object.equals(this.namespace.nil) && moreQuads) {
const node = quads[quads.length - 1].object

quads = quads.concat([...this.dataset.match(node)])
moreQuads = quads.length - moreQuads
}

quads.forEach(quad => {
Expand Down
8 changes: 8 additions & 0 deletions test/Clownface/deleteList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,12 @@ describe('.deleteList', () => {
assert.strictEqual(dataset.size, 1)
assert([...dataset][0].equals(other))
})

it('should not hang if enters in a loop ', () => {
const dataset = rdf.dataset([
rdf.quad(ns.ex.term, ns.ex.p, ns.ex.list)
])
const cf = clownface({ term: ns.ex.term, dataset })
cf.deleteList(ns.ex.p)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test needs a different title and an assertion for when it should succeed

What is the intended outcome? Should ex:p triple be removed?

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right about the misleading title.

My first intention was to just avoid clownface hanging when the user tries to delete a non-list or non-well-formed list. (no ending :nil)

But now that you say it, we must define the desired outcome.

I see some branches of how I Imagine this particular operation could work:

  • If it's a list
    • Clownface performs the operation (1)
  • If it's not a list
    • Clownface throws an error (2)
    • Clownface ignores the operation (3)
  • If it's a malformed list
    • Clownface throws an error (4)
    • Clownface ignores the operation (5)

Could be (1), (3) and (4) a reasonable contract?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree, with a small asterisk

"not a list" is technically a "malformed list" IMO, only such that the firs node if malformed.

<> 
  ex:notList [ ] ;
  ex:malformedList [
    rdf:first "element" ;
    rdf:rest [] ;
  ] .

I would propose a solution where a "malformed list" is ignored when it's already broken on the first node. If the malformation occurs on the rdf:rest predicate, then you'd throw for sure

Copy link
Author

@cristianvasquez cristianvasquez Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So far, so good,

But on second thought, looking into the isList() and list() method at:

clownface/lib/Clownface.js

Line 140 in a31be80

list () {

I think these methods must be reused and their contracts replicated in the deletion operation. (for example, no multiple values for rdf:rest)

Copy link
Author

Choose a reason for hiding this comment

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

@tpluscode I added new border cases to the tests, and the method unfortunately got a bit complex. What do you think about this contract e3a43ff ?

})