Rigorously define equals method on Term and Quad

[RubenVerborgh]

Closes #133.

[elf-pavlik]

👍

[rubensworks]

Looks good to me. Just two minor comments.

[blake-regalia]

See comments

[RubenVerborgh]

Checking for function is extra work; checking for truthy instead of true is less. I don’t mind though; let me know if you want true and I’ll change, I just don’t think it’s necessary to go the extra mile.

[blake-regalia]

Yes, but isn't the idea to always return true or false and not throw an error? What if i pass a term to quad constructor such that quad.term.equals === null? We need to test at runtime that .equals is a function for each component of quad; and that this is another consequence of #132 / #133

[RubenVerborgh]

I don’t feel strongly, will just write true.

[RubenVerborgh]

All of the above comments have been addressed; please re-review as necessary.

[bergos]

+1 for the text, just some small Respec stuff. The comments should be valid for all added id attributes and links.

[blake-regalia]

There is still an outstanding edge case that is not covered by the algorithm which I brought up in my last comment; namely that .equals needs to be tested for or the constructor explicitly needs to convert terms or disallow writing to component properties otherwise what happens when:

factory.quad({
	termType: 'NamedNode',
	value: 'http://ex.org/s',
}, {
	termType: 'NamedNode',
	value: 'http://ex.org/p',
}, {
	termType: 'NamedNode',
	value: 'http://ex.org/o',
}, {
	termType: 'NamedNode',
	value: 'http://ex.org/g',
}).equals(other);

might throw: Uncaught TypeError: {...}.equals is not a function

[RubenVerborgh]

@blake-regalia We are assuming now that they are valid Terms; I can state that assumption somewhere explicitly, but I'd prefer to keep it. Too complex and expensive otherwise.

Also see #137.

[RubenVerborgh]

@bergos' comments processed, pending @blake-regalia response.

[blake-regalia]

We are assuming now that they are valid Terms;
Too complex and expensive otherwise.

@RubenVerborgh By your own logic, iff we are assuming they are valid Terms then we don't need to test for null or undefined! Where is the consistency in that?

[RubenVerborgh]

Null pointer exceptions are a common source of errors, I expect several magnitudes more common than malformed Term exceptions.

[blake-regalia]

Null pointer exceptions are a common source of errors

Agreed - and I still think it would be nice to let devs know that the variable they passed to equals is actually null ;) because who does that intentionally?

I expect several magnitudes more common than malformed Term exceptions.

Well we are not just going for a majority of cases, it needs to be well-defined, consistent and fully-covered.

[RubenVerborgh]

Alright, not disagreeing, but I don't have more bandwidth to follow up on this. Re-assigning to @bergos and @blake-regalia, feel free to conclude in the way you think is most appropriate.

[elf-pavlik]

I think this relates to #104 (Nuance "plain objects (JSON) cannot be used) since @blake-regalia just used such 'plain objects' to make his case.

[vhf]

I added a commit for the first part of the later comments: making .equals parameter optional and nullable.

Removed commit `.equals parameters are optional and nullable`

diff --git a/interface-spec.html b/interface-spec.html
index f0f7b01..5f4cc36 100644
--- a/interface-spec.html
+++ b/interface-spec.html
@@ -129,7 +129,7 @@
     interface Term {
       attribute string termType;
       attribute string value;
-      boolean equals(Term other);
+      boolean equals(optional Term? other);
     };
     </pre>
 
@@ -168,7 +168,7 @@
     interface NamedNode : Term {
       attribute string termType;
       attribute string value;
-      boolean equals(Term other);
+      boolean equals(optional Term? other);
     };
     </pre>
 
@@ -193,7 +193,7 @@
     interface BlankNode : Term {
       attribute string termType;
       attribute string value;
-      boolean equals(Term other);
+      boolean equals(optional Term? other);
     };
     </pre>
 
@@ -222,7 +222,7 @@
       attribute string value;
       attribute string language;
       attribute NamedNode datatype;
-      boolean equals(Term other);
+      boolean equals(optional Term? other);
     };
     </pre>
 
@@ -263,7 +263,7 @@
     interface Variable : Term {
       attribute string termType;
       attribute string value;
-      boolean equals(Term other);
+      boolean equals(optional Term? other);
     };
     </pre>
 
@@ -289,7 +289,7 @@
     interface DefaultGraph : Term {
       attribute string termType;
       attribute string value;
-      boolean equals(Term other);
+      boolean equals(optional Term? other);
     };
     </pre>
 
@@ -333,7 +333,7 @@
       attribute Term predicate;
       attribute Term object;
       attribute Term graph;
-      boolean equals(Quad other);
+      boolean equals(optional Quad? other);
     };
     </pre>
 

(Removed the commit, see it above)

---

As for the other part I'm not sure to understand your suggestion @blake-regalia , let's start here:

In that case, I assume then it would be OK for implementations to throw

Why this assumption? This PR so far doesn't let you throw when you implement .equals, and it specifies that other can be undefined or null, and how to handle the case when it's undefined, null, or a Term/Quad.

Edited: the .equals algo doesn't define what to do when the argument isn't null | undefined | Term but I agree with:

We shouldn't overthink. .quad expects Term instances. If passed something else, behavior is undefined.
#145 (comment) by @RubenVerborgh

(The spec defines the behaviour of .equals(optional Term? other), the behaviour of .equals(anythingElse other) would be undefined behaviour)

@blake-regalia If I understand correctly you suggest explicitly mentioning that this behaviour is undefined?

still discussing on gitter

[bergos]

For me it's ok, but boolean equals(optional Quad? other); would be even better, as it's the exactly what's in the description.

[bergos]

@blake-regalia can you make a proposal what to change so the PR gets your approval?

[elf-pavlik]

On the chat I suggested to reconsider if we really have to define standard (blessed) equal method or we better just leave for libraries to decide how to compare equality and implement it as utility functions: https://gitter.im/rdfjs/Representation-Task-Force?at=5c49de898ce4bb25b802ca8a

@bergos: i will never need a a custom equals, but if some people want a function then they can do it. for me it's just about: const result = functionReturnsTerm(); if (result) { if(quad.predicate.equals(result)) {}} vs. if (quad.predicate.equals(functionReturnsTerm())) {}

@elf-pavlik: vs if (equals(quad.predicate, functionReturnsTerm())) {} where one can choose the right equals() for the job
do we really need that one 'blessed' equals() ?

It also would allow some factories to use 'plain objects' discussed in #104

[elf-pavlik]

As I understand the problem @blake-regalia and @ericprud would like to have possibility to throw if .equals() receives null or undefined while this PR mandates to return false in those cases.

I don't think having MAY or SHOULD helps here since this PR attempts to ensure that equals() will reliably return false for null and undefined.

If we don't define .equals() it would leave it up to implementations to have it as customization and libraries which want to only rely on the spec would need to use a function to compare equality in a way that meets that specific library needs.

[RubenVerborgh]

Closing this concrete proposal until we reach an agreement here: #151

[RubenVerborgh]

Reopening based on resolution of #151.

[elf-pavlik]

@RubenVerborgh with current approvals I think you can just go ahead and merge it.

[blake-regalia]

I added @vhf for review as he had a good suggestion about the signatures.

[blake-regalia]

While we're here, as per @vhf 's suggestion, it might be best to update the signatures as well, i.e., optional Term? other and optional Quad? other. @RubenVerborgh does that sound good to you? If so, let's try and add the commits to this PR.

[RubenVerborgh]

No objection to that.

[elf-pavlik]

@blake-regalia should we just apply patch @vhf posted in his comment above? #142 (comment) and do manual merge?

[blake-regalia]

Okay, made the commits -- let's wait for @vhf to review and then we can merge.