Use PyBaseExceptionRef in places where we expect exceptions - #1644
Conversation
e83be39 to
b8a62b9
Compare
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
Mostly, it looks good to me. But I didn't get much about frame.rs part
|
|
||
| #[pyproperty(name = "__traceback__", setter)] | ||
| fn set_traceback(&self, traceback: Option<PyTracebackRef>, vm: &VirtualMachine) -> PyResult { | ||
| fn setter_traceback(&self, traceback: Option<PyTracebackRef>, vm: &VirtualMachine) -> PyResult { |
There was a problem hiding this comment.
it looks a little bit awkward that getter has get_ prefixed name but setter has setter_ prefixed names
There was a problem hiding this comment.
Yeah I agree, but it was conflicting with the public setter methods so I just renamed them to something that seemed appropriate. I might go with __traceback__/set____traceback__, but that might be too many underscores 😃
There was a problem hiding this comment.
How about adapting #1647 for these methods? This will not fit for everytime but fortunately set_traceback and setter_traceback can share the implementation in this case.
There was a problem hiding this comment.
That sounds good, it would be nice if we could omit the &VirtualMachine parameter too as it's not really necessary, but I'm not sure if that's doable.
| }) | ||
| } | ||
| }; | ||
| let exc_type = if let Ok(exc) = PyBaseExceptionRef::try_from_object(self, exc_type.clone()) |
There was a problem hiding this comment.
I would not create this exc_type variable, but do the condition checking in the code where the exc_type is used below. In fact it might make sense to create another helper function figure_out_exception which returns a PyResult, so we can do:
let exc = figure_out_exception(exc_type, exc_value)?; // figure out a better name for this function ;)There was a problem hiding this comment.
But that function is basically normalize_exception. I've reworked the code; I think it's a lot cleaner and easier to follow now.
windelbouwman
left a comment
windelbouwman
left a comment
There was a problem hiding this comment.
A big change, but it does look like a move in the proper direction. I'm unable to grasp it all though :)
b8a62b9 to
ec3e402
Compare
a6726ee to
181dff8
Compare
|
@windelbouwman would mind reviewing the latest 4 commits? I think I addressed your comments on |
windelbouwman
left a comment
windelbouwman
left a comment
There was a problem hiding this comment.
Looks good to me! I did not see all change, but it looks good I guess!
4 participants
e.g. in
PyResult, the exception stack, and any function that does an operation on an exception. I also noticed a bug where if thefromargument to raise was missing, it would still overwrite the__cause__on the exception as if it had been raisedfrom None.