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

Improve type checking / error when numbers are passed to .throw() #1071

Closed
wants to merge 1 commit into from
Closed

Improve type checking / error when numbers are passed to .throw() #1071

wants to merge 1 commit into from

Conversation

bryanbraun
Copy link

Addresses #1070, and includes a test. Let me know if you have any questions. 😄

Copy link
Contributor

@meeber meeber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @bryanbraun. Few thoughts:

  • I'm fine with type of change as a stopgap measure, since it makes the error message more clear for an operation that already doesn't work. As mentioned in the last bullet point in WIP: Update .throw and add .error #1021, I'd like to eventually tighten up this assertion even more in terms of what types are allowed, although some of the changes I have in mind would be breaking.
  • The change in this PR tackles the reported problem of expect(() => { throw 10}).to.throw(10); giving an unhelpful error message, but it leaves behind some similar problems due to .throw accepting multiple parameters and behaving differently based on the types of parameters. For example: expect(() => { throw Error(10)}).to.throw(Error, 10); would still give a confusing error message
  • Regardless of final solution, we'll want similar tests for the expect and should interfaces as well

@bryanbraun
Copy link
Author

@meeber, thanks for the feedback. I'll add tests for those other interfaces, and work on handling those multi-parameter cases as well. 👍

Copy link
Member

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi folks! Thanks for the PR @bryanbraun (btw I'm already missing southern California's burritos, we must definitely go back for some 😄 )!

I agree with @meeber regarding the tests on all interfaces and his point about the message being confusing when writing expect(() => { throw Error(10)}).to.throw(Error, 10).

In order to fix that, a check similar to what you have already added could be placed around here providing a useful message when errMsgMatcher is an instanceof a Number.

I'm looking forward to merging this once the PR meets this requisite.

Thanks buddy!

@bryanbraun
Copy link
Author

(I was going to update this but I saw the other work happening in #1021, so I'm holding off for now and keeping an eye on that one. If that doesn't end up working out, I'm happy to jump back in here and make the changes that were recommended)

@keithamus
Copy link
Member

Hey @bryanbraun! Thanks for making this PR. I'm sitting next to @lucasfcosta right now and we've been discussing where to go with this PR. While we're keen to get this merged, so we can register your contribution to chai - the reality is we're not going to be making a release any time soon, until we've cleaned up a lot of stuff. So merging this would just give a false sense that we're going to release it - but that won't be true. This feature will eventually land in chai 5, but chai 5 will be a while off.

Please don't take that as a sign that your contribution wasn't worthwhile! We hugely appreciate the time you've put into this and please, don't let this dissuade you from making more PRs! I really hope we see more contributions from you 😃

@keithamus keithamus closed this Jun 9, 2018
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

Successfully merging this pull request may close these issues.

5 participants