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.

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

Usage of exceptions/errors? #132

Open
k00ni opened this issue Nov 24, 2018 · 21 comments
Open

Usage of exceptions/errors? #132

k00ni opened this issue Nov 24, 2018 · 21 comments

Comments

@k00ni
Copy link
Contributor

k00ni commented Nov 24, 2018

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
Copy link
Member

bergos commented Nov 27, 2018

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
Copy link
Contributor

blake-regalia commented Nov 27, 2018

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
Copy link
Contributor Author

k00ni commented Nov 28, 2018

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
Copy link
Contributor Author

k00ni commented Nov 28, 2018

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
Copy link
Member

RubenVerborgh commented Nov 28, 2018 via email

@blake-regalia
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

blake-regalia commented Nov 28, 2018

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
Copy link
Member

bergos commented Nov 28, 2018

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
Copy link
Contributor

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
Copy link
Member

RubenVerborgh commented Nov 28, 2018

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

k00ni commented Nov 29, 2018

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
Copy link

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

awwright commented Jan 22, 2019 via email

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

No branches or pull requests

7 participants