-
Notifications
You must be signed in to change notification settings - Fork 122
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
v8pp throws strings by default, which is bad practice #205
Comments
Hi Jean-Claude, You have proposed the right thing, thanks 👍. A possible fix would be just using Sure, PR is welcomed! |
I pushed a PR. In my own binding code I use following helper functions (which originates from the time where v8pp didn't even have the option to specify an exception type).
Given there are only a handful of exception types, a similar API which could be considered. The current Admittedly, the current API with the ctor function pointer is fine too. |
I like your idea, to have a set of functions for throwing particular exception types. Do you mean to have them one-liners that call inline v8::Local<v8::Value> throw_error(v8::Isolate* isolate, std::string_view message, v8::Local<v8::Value> exception_options = {})
{
return throw_ex(isolate, message, v8::Exception::Error, exception_options);
}
inline v8::Local<v8::Value> throw_type_error(v8::Isolate* isolate, std::string_view message, v8::Local<v8::Value> exception_options = {})
{
return throw_ex(isolate, message, v8::Exception::TypeError, exception_options);
}
|
Yes, exactly. |
like `throw_error()`, `throw_range_error()` see #205
v8pp::throw_ex
throws a string by default, which is considered a bad practice by many:https://stackoverflow.com/questions/11502052/throwing-strings-instead-of-errors
In the application I'm working on, errors from user scripts are formatted in a standard way including stack traces, but it fails if the error is thrown within v8pp. The user is left without stack trace. For that reason I have a patch to default to throwing and
Error
instead of a string in v8pp.Would this patch be welcome upstream?
The text was updated successfully, but these errors were encountered: