-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[cdnjs gem] make exception classes more consistent #1683
Changes from 3 commits
9e93616
734bfe1
ce77f48
8c5168c
3161670
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,60 @@ | ||
'use strict'; | ||
|
||
class NotFound extends Error { | ||
constructor(prettyMessage = 'not found') { | ||
class ShieldsRuntimeError extends Error { | ||
|
||
get name() { return 'ShieldsRuntimeError'; } | ||
get defaultPrettyMessage() { throw new Error('Must implement abstract method'); } | ||
|
||
constructor(props, message) { | ||
props = props || {}; | ||
super(message); | ||
this.prettyMessage = props.prettyMessage || this.defaultPrettyMessage; | ||
if (props.underlyingError) { | ||
this.stack = props.underlyingError.stack; | ||
} | ||
} | ||
} | ||
|
||
class NotFound extends ShieldsRuntimeError { | ||
|
||
get name() { return 'NotFound'; } | ||
get defaultPrettyMessage() { return 'not found'; } | ||
|
||
constructor(props) { | ||
props = props || {}; | ||
const prettyMessage = props.prettyMessage || 'not found'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this line (& line below) be using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be ideal, but there's a catch 22: If we try to reference Maybe an alternative would be to declare the default messages as constants outside the classes so we can say const defaultNotFoundMessage = 'not found';
class NotFound extends ShieldsRuntimeError {
get name() { return 'NotFound'; }
get defaultPrettyMessage() { return defaultNotFoundMessage; }
constructor(props = {}) {
const prettyMessage = props.prettyMessage || defaultNotFoundMessage;
//...
}
} do you think that is better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah that makes sense, sounds good to me 👍 |
||
const message = prettyMessage === 'not found' | ||
? 'Not Found' | ||
: `Not Found: ${prettyMessage}`; | ||
super(message); | ||
this.prettyMessage = prettyMessage; | ||
this.name = 'NotFound'; | ||
super(props, message); | ||
} | ||
} | ||
|
||
class InvalidResponse extends Error { | ||
constructor(prettyMessage = 'invalid', underlyingError) { | ||
const message = underlyingError | ||
? `Invalid Response: ${underlyingError.message}` | ||
class InvalidResponse extends ShieldsRuntimeError { | ||
|
||
get name() { return 'InvalidResponse'; } | ||
get defaultPrettyMessage() { return 'invalid'; } | ||
|
||
constructor(props) { | ||
props = props || {}; | ||
const message = props.underlyingError | ||
? `Invalid Response: ${props.underlyingError.message}` | ||
: 'Invalid Response'; | ||
super(message); | ||
this.stack = underlyingError.stack; | ||
this.prettyMessage = prettyMessage; | ||
this.name = 'InvalidResponse'; | ||
super(props, message); | ||
} | ||
} | ||
|
||
class Inaccessible extends Error { | ||
constructor(underlyingError, prettyMessage = 'inaccessible') { | ||
super(`Inaccessible: ${underlyingError.message}`); | ||
this.stack = underlyingError.stack; | ||
this.prettyMessage = prettyMessage; | ||
this.name = 'Inaccessible'; | ||
class Inaccessible extends ShieldsRuntimeError { | ||
|
||
get name() { return 'Inaccessible'; } | ||
get defaultPrettyMessage() { return 'inaccessible'; } | ||
|
||
constructor(props) { | ||
props = props || {}; | ||
const message = props.underlyingError | ||
? `Inaccessible: ${props.underlyingError.message}` | ||
: 'Inaccessible'; | ||
super(props, message); | ||
} | ||
} | ||
|
||
|
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.
would we be better to remove this line and declare the default value in the header instead?
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.
👍 updated