search

thejerf / suture

Supervisor trees for Go.
MIT License
1.32k stars 74 forks source link

Handle non-string panic values. #60

Closed lthibault closed 2 years ago

lthibault commented 2 years ago

Fixes: https://github.com/thejerf/suture/issues/59

⏱️ Estimated Review Time: 5 minutes ✅ Merge when ready

lthibault commented 2 years ago

@thejerf In an ideal world, I would have renamed EventServicePanic.PanicMsg to PanicVal and changed its type to interface{}, but this is the next best thing. As far as I can tell, this should preserve backwards compatibility. But let me know if I'm overlooking some kind of corner-case.

thejerf commented 2 years ago

I agree but I can't justify the backwards compatibility break, I think.

If it's configurable then at least you can JSON it yourself if you want to or something.

lthibault commented 2 years ago

I agree but I can't justify the backwards compatibility break, I think.

If it's configurable then at least you can JSON it yourself if you want to or something.

Sorry, I think I was unclear: this PR aims to preserve backwards compatibility, i.e. it does not change EventServicePanic.

(Edit: nvm - I guess the code clarified things 🙂 )

thejerf commented 2 years ago

Yes, sorry, I was unclear myself. I was agreeing about your perfect world scenario.

Well, it's a good reason to roll a new version. There's been a couple of open issues here that I didn't quite want to roll a new version for but the sum total is worth it. Give me ~1 hour here and you should have a nice solid v4.0.2 tag to work with.

thejerf commented 2 years ago

v4.0.2 is out. I think it should be safe, tests all pass, but you know this is tricky code. If anything goes wrong, you know where to find me.

lthibault commented 2 years ago

Will do! Many thanks once again for your help with this.