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

Add a way for js_error! macro to create native errors with message #3971

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Aug 26, 2024

I assume this will have a fair bit of bikeshedding on the interface.

This only supports text errors since it only adds the message (which is text only). Supports syntax similar to format!(), e.g. js_error!(TypeError: "the number {} should be even", odd_number).

Other Considerations and Explanation

I considered another option for the API: having a separate macro per type (like js_typ_error!(), js_syntax_error!(), etc). I decided to dismiss this and went the way of this PR instead as the code looks closer to what the message will be, since it's normally prefixed with the error. It also declutters the use statements as there's only one macro.

For example, the following code:

js_error!(TypeError: "not a day of the week")

would show the following message when printed on the console:

TypeError: not a day of the week

Tested with boa_cli:
CleanShot 2024-08-26 at 16 37 00@2x

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.86%. Comparing base (6ddc2b4) to head (804c209).
Report is 256 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3971      +/-   ##
==========================================
+ Coverage   47.24%   51.86%   +4.61%     
==========================================
  Files         476      468       -8     
  Lines       46892    45293    -1599     
==========================================
+ Hits        22154    23490    +1336     
+ Misses      24738    21803    -2935     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nekevss nekevss added discussion Issues needing more discussion enhancement New feature or request API labels Aug 28, 2024
@nekevss nekevss requested a review from a team August 28, 2024 17:39
@nekevss
Copy link
Member

nekevss commented Aug 28, 2024

Assuming I'm reading this right. I think my biggest concern is I'm not sure I'm thrilled about the case when args are needed for format.

Without format:

    let err = js_error!(TypeError: "{} was not a string", value.to_string());

With format:

    let err = js_error!(TypeError: format!("{} was not a string", value.to_string()));

But after looking at it, I don't like the version with format either. Overall, I think this looks pretty nice. Out of curiosity, was there any other formats that you had tried?

@hansl
Copy link
Contributor Author

hansl commented Aug 31, 2024

I tried having one macro by type of native error. It wasn't saving that many stroke and rustfmt does the right thing here so I went for the simpler import.

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

I think the current interface is fine.
Personally, I'm not a huge fan of macros in general but if there is a macro to create an error, it should be only one instead of one per native type.

@raskad raskad added this to the next-release milestone Sep 10, 2024
@raskad raskad requested a review from a team September 10, 2024 23:32
Copy link
Member

@nekevss nekevss 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 fine with moving forward with this as is.

@nekevss nekevss added this pull request to the merge queue Sep 11, 2024
Merged via the queue into boa-dev:main with commit affa652 Sep 11, 2024
14 checks passed
hansl added a commit to hansl/boa that referenced this pull request Sep 12, 2024
…oa-dev#3971)

* Add a way for js_error! macro to create native errors with message

* Fix docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API discussion Issues needing more discussion enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants