-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add error predicate #27
Conversation
source/lib/predicates/error.ts
Outdated
} | ||
|
||
/** | ||
* Test an Error to an EvalError. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing be
here and in the other descriptions.
source/lib/predicates/error.ts
Outdated
/** | ||
* Test an Error to a TypeError. | ||
*/ | ||
get typeError() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should put this above evalError
as it's the most common Error subclass and should be sorted first of subclasses in autocomplete.
source/lib/predicates/error.ts
Outdated
* | ||
* @param instance The expected instance type of the error. | ||
*/ | ||
private addErrorTypeValidator(instance: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your strategy for method sorting? I usually put private method/props first as they're usually used by the public ones and therefore it's nice to read their logic first so you know how they work before you encounter calls to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right. Didn't thought about it and refactored some things last minute. Private-first makes sense.
How should we handle the Object predicate? For example, here we could also use a |
4589246
to
8ea367c
Compare
Not sure if I understand you correctly. So what you want is to add something like It could/would look like this. abstract class InstanceOfPredicate<T> extends Predicate<T> {
constructor(type: string, context?: Context) {
super(type, context);
}
instanceOf(instance: any) {
return this.addValidator({
message: obj => `Expected \`${obj.name}\` to be an instance of \`${instance.name}\``,
validator: obj => obj instanceof instance
});
}
}
class ErrorPredicate extends InstanceOfPredicate<Error> {
constructor(context?: Context) {
super('error', context);
}
}
class ObjectPredicate extends InstanceOfPredicate<Object> {
constructor(context?: Context) {
super('object', context);
}
} Not sure about this. It deduplicates code, but it's only a small piece and it doesn't allow you to specify a custom method description. Something like |
No, I meant that errors are objects, so it might make sense for errors to inherit the object predicates, but thinking about this a bit more I don't think it makes sense as the error message would be wrong and not all object predicates makes sense for error. Let's just add the |
It's common to have custom properties on errors, so maybe we should add a |
I took a look for class CustomError extends Error {
constructor(message: string) {
super(message);
this.name = 'CustomError';
}
}
const err = new CustomError('foo');
console.log(err instanceof CustomError); // false
console.log(err instanceof Error); // true This works in ES2015 though, but apparently this is very difficult to transpile in ES5 code. So currently, not sure if we should add an |
source/lib/predicates/error.ts
Outdated
hasKeys(...keys: string[]) { | ||
return this.addValidator({ | ||
message: () => `Expected error message to have keys \`${keys.join('`, `')}\``, | ||
validator: error => keys.every(key => (error as Object).hasOwnProperty(key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use as Object
? Errors are objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. I thought I had to do this at first because I didn't get any intellisense without the casting. But it works fine without as well.
This is just one of many issues we have to deal with because we compile to ES5. I think we instead should compile to ES6 and use Babel to transpile the remaining things to get Node.js 4 compat. What do you think? |
I was actually thinking: class CustomError extends Error {
constructor(message: string) {
super(message);
this.name = 'CustomError';
}
}
const isType = (err, constructor) => Object.getPrototypeOf(err) === constructor.prototype;
console.log(isType(new CustomError('x'), CustomError)); So not instance of, but rather testing the type directly. Or is it better to just use instanceof? |
It would let us check if something is a custom Error. |
Yes I totally agree, if it works correctly :) which it doesn't with The
If we go down this road, we might reconsider support for Node.js 4 as it gets end of life in April 2018. Not sure how easy it is to set this up though, never really used Babel a lot. |
Sorry, I should have been clearer. I know that's not working right now either. I just meant to ask if that's how we should do it if we fix the inheritance problem.
Yeah, maybe we should just target Node.js 6 and we don't have to use Babel at all. I think we should. |
Done: 474139f |
Do you think we should have a check for any inheritance (instanceof) or direct inheritance? |
57f3e35
to
3dca3fd
Compare
Not really sure to be honest. If we name the method |
I removed the private method and re-used |
🙌 |
🎉 Feedback welcome!