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

Custom Exceptions are confusing #280

Closed
tejasbubane opened this Issue May 8, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@tejasbubane
Copy link
Member

tejasbubane commented May 8, 2017

I was solving Circular Buffer today and had a hard time figuring out how to create custom exceptions. That is because I was expecting the matcher to match with a custom exception class, but what it actually does is matches with Error itself.

expect(()=>buffer.read()).toThrow(bufferEmptyException());

Note bufferEmptyException is a function call which gives no idea what its return type should be - Error/CustomError/String??

For custom exceptions in ES6, most sources say that this should suffice: (which is far better than its ES5 equivalent)

class bufferFullException extends Error {}

But it does not work since babel does not allow subclassing built-in classes (also mentioned in the docs). I ended up using the babel-plugin for allowing subclassing. Solution here.

Most other exercises just throw a String, while others like wordy and circular-buffer throw custom exceptions.
So I have two questions:

  • Should we have a standard way of doing exceptions in the track? Is throwing Strings OK or should we go all custom?

  • For the exercises that expect custom errors, is class bufferFullException extends Error {} better than what we have right now? which is:

const BufferFullException = () => ({
    name: "buffer full exception!",
    message: "can't write to a full buffer!"
  });
@matthewmorgan

This comment has been minimized.

Copy link
Contributor

matthewmorgan commented May 8, 2017

@tejasbubane I think there's value in revealing this feature of the language. Your work on #279 moved us from throwing a string to throwing an actual Error object, and I feel like this is another step in the right direction.

I am wondering if the plugin you mentioned could be part of our current Jest pipeline?

@tejasbubane

This comment has been minimized.

Copy link
Member

tejasbubane commented May 8, 2017

I feel like this is another step in the right direction.

Glad you think the same. :)

I am wondering if the plugin you mentioned could be part of our current Jest pipeline?

Yes, we need to add the plugin as dependency and add this to babel settings in package.json:

    "plugins": [
      ["babel-plugin-transform-builtin-extend", {
        "globals": ["Error"]
      }]
    ]

I think it is safe since it allows only the Error builtin class to be extended and not others like Array, etc. And plays nice with Jest since we can now change this:

const BufferEmptyException = () => ({
  name: "buffer empty exception!",
  message: "can't read from an empty buffer!"
});

expect(()=>buffer.read()).toThrow(bufferEmptyException());

to this:

export class BufferEmptyException extends Error {};

expect(()=>buffer.read()).toThrow(BufferEmptyException);

Note: No function call, assertion on class. ☝️

I will open a PR and we can move the detailed discussion there.

tejasbubane added a commit to tejasbubane/xjavascript that referenced this issue May 8, 2017

tejasbubane added a commit to tejasbubane/xjavascript that referenced this issue May 8, 2017

tejasbubane added a commit to tejasbubane/xjavascript that referenced this issue May 8, 2017

tejasbubane added a commit to tejasbubane/xjavascript that referenced this issue May 10, 2017

tejasbubane added a commit to tejasbubane/xjavascript that referenced this issue May 10, 2017

@rchavarria

This comment has been minimized.

Copy link
Contributor

rchavarria commented May 10, 2017

Awesome! That would be really good! That's a language feature users will learn on exercism.io.

Should we have a standard way of doing exceptions in the track?

I think so. I don't mean to change all thrown Errors to custom exceptions, but at least, throw some kind of Error, not strings.

Canonical data for some exercises (I can't think of an example here) use -1 or other weird values to tell that an exception could be thrown. I think we could change those exercises to throw errors/exceptions instead of checking -1 or other flag values.

tejasbubane added a commit to tejasbubane/xjavascript that referenced this issue Jun 2, 2017

southp added a commit to Automattic/wp-calypso that referenced this issue Nov 9, 2017

Enable subclassing of Error by introducing the
`babel-plugin-transform-builtin-extend` plugin, so that we can write
test like `expect( func ).toThrow( CustomError ). More details here:
exercism/javascript#280

mattwondra added a commit to Automattic/wp-calypso that referenced this issue Nov 10, 2017

Enable subclassing of Error by introducing the
`babel-plugin-transform-builtin-extend` plugin, so that we can write
test like `expect( func ).toThrow( CustomError ). More details here:
exercism/javascript#280

southp added a commit to Automattic/wp-calypso that referenced this issue Nov 14, 2017

Concierge Chats: transform and validate the API response from /concie…
…rge/schedules/{id}/shifts. (#19621)

* Add the api response transformer and the validator for
`/concierge/schedules/{id}/shifts`

* Enable subclassing of Error by introducing the
`babel-plugin-transform-builtin-extend` plugin, so that we can write
test like `expect( func ).toThrow( CustomError ). More details here:
exercism/javascript#280

* Remove the use of `lodash.get`, since it's not necessary here.

* Fix the typo and add the missing test suite.

* Enable transform-extend-builtin only in test

* Mark `begin_timestamp`, `end_timestamp` and `schedule_id` as required.

* Add a test for `transformShift`

rclations added a commit to rclations/wp-calypso that referenced this issue Nov 15, 2017

Concierge Chats: transform and validate the API response from /concie…
…rge/schedules/{id}/shifts. (Automattic#19621)

* Add the api response transformer and the validator for
`/concierge/schedules/{id}/shifts`

* Enable subclassing of Error by introducing the
`babel-plugin-transform-builtin-extend` plugin, so that we can write
test like `expect( func ).toThrow( CustomError ). More details here:
exercism/javascript#280

* Remove the use of `lodash.get`, since it's not necessary here.

* Fix the typo and add the missing test suite.

* Enable transform-extend-builtin only in test

* Mark `begin_timestamp`, `end_timestamp` and `schedule_id` as required.

* Add a test for `transformShift`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment