Usage of exceptions/errors?

k00ni commented 5 years ago

The specification currently doesn't specific certain errors or exceptions. Is that intended? Would you open to define ones for certain cases?

One would be if someone tries to change an instance of Term or Quad. But perhaps the decision was already made against that (ref #81).

Another one, if you use the wrong Term subclass in a quad, e.g. predicate = BlankNode.

(ref #130)

bergos commented 5 years ago

Both cases are not handled by the spec. Also in JS it's not very common to make custom error classes. And instanceof should not be used, as this could cause problems with different implementations.

blake-regalia commented 5 years ago

I think this is important for all methods. For instance, if null is passed to NamedNode.equals(), should this throw an Error or return false? Right now, the org data model test suite requires these methods return false but this is not defined in the interface specification -- I recommended we either remove this from the test suite or define it in the interface spec.

k00ni commented 5 years ago

I'd rather prefer clear exceptions or just errors, but with defined error texts, instead of returning false. In a complex environment, a returned false says nothing about the state of the previous operation. Giving the user a way to gather critical information is important, if you handle invalid data, while parsing for instance.

k00ni commented 5 years ago

As a starting point, i propose the following exceptions:

InvalidTermUsageInQuadException

Throw if: you use an invalid instance of Term in Quad.
Example: use a Literal instance as subject
Texts:
- invalid subject: Subject can only be one of: BlankNode, NamedNode, Variable
- invalid predicate: Predicate can only be one of: NamedNode, Variable
- invalid object: Object can only be one of: BlankNode, Literal, NamedNode, Variable
- invalid graph: Graph can only be one of: BlankNode, DefaultGraph, NamedNode, Variable

ObjectNotMutableException

Throw if: you try to change an instance of Term or Quad.

UndefinedPropertyException

Throw if: you try to access an undefined property of an instance of Term or Quad. Predefined are subject, predicate, object and graph in Quad and value, termType in Term.

RubenVerborgh commented 5 years ago

2 is not idiomatic; 3 not enforceable on regular objects (except for proxies)

blake-regalia commented 5 years ago

I think the best we can hope for would be to say that certain methods should throw an Error, maybe even for example a TypeError, if provided an argument that is not a Term, Quad, etc. This is critical for debugging if a variable in a dev's code is not properly initialized and passes undefined to say, .equals(term). Returning false instead of throwing is not a convenience, it only further obscures the source of such problems.

RubenVerborgh commented 5 years ago

I disagree. A false for me is expected behavior; I asked whether the thing is equal and the answer is clearly no.

Debugging is not hard: the equals method will return false, so that's not harder to debug than an actual Term that would also be unequal.

blake-regalia commented 5 years ago

A false for me is expected behavior

Not for me... I'd expect equality to be symmetric

let term = factory.namedNode('a');
let undef;
term.equals(undef);  // false
undef.equals(term);  // > TypeError: Cannot read property 'equals' of undefined

RubenVerborgh commented 5 years ago

Good point about symmetry, but I assume you don't want TypeError: Cannot read property 'equals' of undefined for term.equals(undef), so how much is symmetry worth 😉

blake-regalia commented 5 years ago

The symmetry is that they both would throw an error, whether or not the return value from the .toString() method of one error is identical to the other is not important. What's important about symmetry is that switching which side of the operator a variable is on does not cause different effects on the rest of a program, namely branching as a result of false if term is on the left rather than on the right.

bergos commented 5 years ago

I'm against throwing an error. Maybe in TypeScript, but it's not very "JSish". In JS I don't expect any type error. "not null" or "no empty string" etc. is OK, but type checks are so blurry in JS. Think of this case:

const term = factory.namedNode('a')
const other = {}

term.equals(other) // false, cause term.termType !== other.termType

blake-regalia commented 5 years ago

The bigger point is that we are doing nobody any favors by returning false for a falsy other argument. I guarantee that nobody is intentionally testing if term.equals(null) or term.equals(false) or term.equals(undefined) or term.equals(0) or term.equals('') or term.equals(). If the developer is even aware of the possibility that the value of their argument to .equals for example might be falsy, they will do a falsy check on their own anyway before calling .equals. Nobody is going to think "ahh - how convenient, I can just pass this possibly undefined value to .equals without checking for falsyness first"

RubenVerborgh commented 5 years ago

The bigger point is that we are doing nobody any favors by returning false for a falsy other argument.

Me; I don't have to write a catch handler ever.

Nobody is going to think "ahh - how convenient, I can just pass this possibly undefined value to .equals without checking for falsyness first"

That would be me.

I've just always written equals methods to return booleans and not throw; I think this stems from the Java definition of it, which does not throw under any circumstance.

We do not have a lot to go in in JavaScript-land though, except for Object.is.

My notion of equals seems to be the same as that of Object.is, but I don't have much evidence regarding whether or not throwing for falsy values (BTW why not for truthy?) is idiomatic. I see exceptions for ways where methods cannot give a proper answer, I think for equals there always is (depending on your exact definition of it, of course).

elf-pavlik commented 5 years ago

I don't have to write a catch handler ever.

To assume that equals() will not throw, does spec need to mandate MUST NOT to throw? Besides saying nothing, we have at least option to say MUST NOT, MAY, SHOULD, MUST. Depending on that implementations could make different assumptions about behavior (including not to assume anything about throwing from equals())

blake-regalia commented 5 years ago

My notion of equals seems to be the same as that of Object.is, but I don't have much evidence regarding whether or not throwing for falsy value.

...you might have just finally convinced me 🙂

We do not have a lot to go in in JavaScript-land though, except for Object.is.

Actually, Number.isFinite, Number.isInteger, Number.isSafeInteger, Number.isNaN, String.prototype.startsWith and String.prototype.includes (perhaps the most important one for this case) all return false for falsy values (including no argument calls) instead of throwing TypeErrors. I'd say this sets the precedent in favor of returning false. The reason I find this so compelling is that String.prototype.includes is an instance method that expects another instance of the same class, the same circumstances as Term.prototype.equals except for the symmetry aspect.

k00ni commented 5 years ago

Good morning,

i also would prefer not to throw anything when using equals. Returning false is enough if something fishy was given.

@RubenVerborgh wrote:

I see exceptions for ways where methods cannot give a proper answer, [...]

That's the point. Setting an invalid value for predicate in Quad is such an example (1). Or if one tries to change an immutable object (2). How should the system react? Or from a developers point: how does a developer expect it to react?

(1) for me is clearly an invalid state. The program should not ignore that, because it violates the specification. If you accept that, than why care about these statements in the specification?

§2.8 Quad interface subject the subject, which is a NamedNode, BlankNode or Variable. predicate the predicate, which is a NamedNode or Variable. object the object, which is a NamedNode, Literal, BlankNode or Variable. graph the named graph, which is a DefaultGraph, NamedNode, BlankNode or Variable.

Case (2) could go either way. Throwing or ignore the change.

I would like to rephrase my initial question: In which cases are we reaching an invalid state and can't process with the given input?

Thanks for the responses.

ericprud commented 5 years ago

My life as a developer is much easier if the libraries help me catch errors as soon as they occur, rather than having to gradually back up in a debugger from a pathological consequence much later on. An uncaught exception tells me immediately that I invoked a function with bad parameters; I find and fix those bugs so readily I don't really notice them.

To Ruben's point of not having to write catch clauses, I don't think a well-behaved code path would require catch clauses for the functions we're considering; it would require the same defensive programming you'd need if erroneous invocations returned false, but you'd have a much easier time tracking down the places where you accidentally passed a null or undefined. Applications that requires an outer catch, e.g. to send an error back in an HTTP response, would probably already need to do that to catch the myriad ways TypeError may be thrown.

blake-regalia commented 5 years ago

An uncaught exception tells me immediately that I invoked a function with bad parameters; I find and fix those bugs so readily I don't really notice them.

I totally agree with this, the dangers of passing around a value that you believe is a Term or a Quad but is actually not defined are far reaching -- however, there seems to be strong precedent in JavaScript for returning false with class method calls when they are passed an instance of their same class, i.e., other, for example String.prototype.includes. Furthermore, the consensus on the call is that equals should always return true of false no matter the type. For these reasons, we are concluding that errors will not be thrown.

awwright commented 5 years ago

there seems to be strong precedent in JavaScript for returning false with class method calls when they are passed an instance of their same class

It appears to me ECMAScript returns false when the method is a question, as opposed to an operation in general. For example, (1).toString(false) returns a RangeError, as that's outright nonsensical.

I throw an error for malformed IRIs in constructors, for example—as that's clearly nonsensical. (And this could be implemented as an assertion that can be disabled in production.) For questions like equals I just return false: Asking if a NamedNode is a boolean may be odd, but it's not nonsensical.

And actually, in my implementation, there's an option to treat protypical values (string, etc) as a Literal. So it's not all that odd to ask my function literal("0","http://www.w3.org/2001/XMLSchema#boolean").equals(false)—the answer there is true.

I'm not sure if we need to treat this in the text though. There might be a legitimate reason to have it one way or the other for different contexts. As long as implementations right now seem to be doing the right thing, which I think includes a preference to throw as early as possible.

elf-pavlik commented 5 years ago

And actually, in my implementation, there's an option to treat protypical values (string, etc) as a Literal. So it's not all that odd to ask my function literal("0","http://www.w3.org/2001/XMLSchema#boolean").equals(false)—the answer there is true.

I think this implementation does not conform to #142 and if code relying on it gets a Literal produced by different data factory it will get answer false

awwright commented 5 years ago

Enabling this deeply modifies the built-in prototypes; it's sketchy behavior to say the least, but it descends from webr3's js3 library and is intended for environments that want to deeply embed RDF semantics the same way Node.js has a standard Buffer implementation.

On Tue, Jan 22, 2019, 09:17 elf Pavlik <notifications@github.com wrote:

And actually, in my implementation, there's an option to treat protypical values (string, etc) as a Literal. So it's not all that odd to ask my function literal("0","http://www.w3.org/2001/XMLSchema#boolean ").equals(false)—the answer there is true.

I think this implementation does not conform to #142 https://github.com/rdfjs/representation-task-force/pull/142 and if code relying on it gets a Literal produced by different data factory it will get answer false

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rdfjs/representation-task-force/issues/132#issuecomment-456460570, or mute the thread https://github.com/notifications/unsubscribe-auth/AAatDSXwpdPXv0HuWzMrnzR1mFNBTiyhks5vFzmZgaJpZM4YxgOF .

rdfjs / data-model-spec