Return message string from model panic handler to be displayed in UI … #1534
Conversation
There was a problem hiding this comment.
Thank for you for this pull request, @mxdamien!
I think this is a good start. The main cause of panics, the user-written model code, is taken care of. However, there are some problems around the periphery. I've left some comments with my thoughts. Please let me know what you think!
Also, and this is more a note to myself, here's what a todo!() in model code looks like, as of this pull request:
I think this is good enough for now, but eventually we would want an error message that makes clear that the error originated from a panic in the model code, and add as much detail as possible (file name, line, etc.).
None of this needs to be a part of this pull request. I'm just making a note, so I remember to open an issue, once this is merged.
|
Thanks for the review Hanno. I'll see what I can do and come back with questions for sure ;)
|
|
Thank you, David! |
mxdamien
force-pushed
the
feat/ShowStatusInUIAfterPanicInModel
branch
from
9078292
to
bbd03e8
Compare
mxdamien
force-pushed
the
feat/ShowStatusInUIAfterPanicInModel
branch
9 times, most recently
from
0f0ddc2
to
17cfef2
Compare
mxdamien
force-pushed
the
feat/ShowStatusInUIAfterPanicInModel
branch
from
17cfef2
to
c89b948
Compare
|
Hi ! I hope I've addressed your remarks. I'm not entirely sure about the usage of the different internal / ffi-boundary structs. It's my first time dealing with this kind of stuff in Rust. David |
|
Thank you for keeping at it, David! I'll take a look later today. |
There was a problem hiding this comment.
Thank you, @mxdamien! This is looking great!
I've left one comment about a thing I don't like, but I decided to merge the pull request anyway. Panics in the model code are properly handled now, and I believe that the error being swallowed there is a corner case.
I'm going to open an issue to track that. Feel free to follow up with another pull request to address it, if you want.
| false => Some(other.into_str()), | ||
| } | ||
| } | ||
| super::ffi_safe::Result::Err(_) => None, |
There was a problem hiding this comment.
It's not ideal to swallow the error here, as that could cause bugs to be hidden. Instead of getting an error message, making it clear that a bug has occurred, it could lead to a specified argument just not being recognized, which would be very confusing.
I think Context::get_argument should also return Result instead of an Option.
|
I've opened the following issues in response to this pull requests: |
…status
closes #1519