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

Added "type found" msg to expose_assert_* fns #3766 #3787

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

rimutaka
Copy link
Contributor

This PR prints out the type of the argument encountered when the type assertion fails as discussed in #3766

  • old msg: expected a number argument
  • mew msg: expected a number argument, found string

The core change

  • added , found ${typeof(n)} to int, str and bool assertions

Related changes

Simple tests did not have anything for boolean type assertion, so I added one because this PR changes the bool assertion and needs a test.
Was it included elsewhere and mine is a duplicate?

Only negative assertions were tested. E.g. passing a string to a number arg.
I added positive tests as well. E.g. passing a number to a number arg.
Maybe there was a reason why they were omitted?

* added `, found ${typeof(n)}` to int, str and bool assertions
* added assertion test for bool
* added positive test for int, str and bool assertions
@rimutaka rimutaka marked this pull request as ready for review January 16, 2024 02:52
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

I'm not sure if we had any tests in place somewhere else or why some tests where omitted. But my guess is that a lot of tests are just missing and I'm happy that you added those.

Thank you!

@daxpedda daxpedda merged commit 55a7fb8 into rustwasm:main Jan 16, 2024
25 checks passed
aDogCalledSpot pushed a commit to aDogCalledSpot/wasm-bindgen that referenced this pull request Jan 16, 2024
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.

2 participants