You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is something that I missed when implementing #541 (sorry! 馃檹) that may be annoying to people wanting to use the local contexts: If we pass a block to Honeybadger.context I would expect the return value to be the result of the block, rather than the Honeybadger::Agent instance.
Imagine a case we have a critical payments-related process in our application and we want to make sure that all errors triggered in this process are tagged. Pseudocode:
classSuperImportantPaymentProcessingdefcall# bunch of things happening{success: true,charge: }endend
With the current implementation we need to either create additional variable before the block to make sure the return value from #call remains the same:
classSuperImportantPaymentProcessingdefcallresult=nilHoneybadger.context({tags: ['critical','payments']})do# bunch of things happeningresult={success: true,charge: }endresultendend
or we have to use return explicitly:
classSuperImportantPaymentProcessingdefcallHoneybadger.context({tags: ['critical','payments']})do# bunch of things happeningreturn{success: true,charge: }endendend
While it still works, it feels unexpected. I'm not sure if there's any actual API level expectation that Honeybadger.context returns Agent instance (it's undocumented which is why I missed it) but I think it could make sense to modify the behaviour. I just noticed that the reason it returns Agent instance is to allow chaining, e.g. Honeybadger.context.clear!. I think it makes sense without the block but I would still modify the behaviour for the block version.
Having the behaviour modified would also pair with rescue blocks well, e.g.
classSuperImportantPaymentProcessingdefcallHoneybadger.context({tags: ['critical','payments']})do# bunch of things happening{success: true,charge: }rescuePaymentError=>eHoneybadger.notify(e)# this still gets tagged!{success: false}endendend
Let me know what you think and I can prep the PR with a fix.
The text was updated successfully, but these errors were encountered:
This is something that I missed when implementing #541 (sorry! 馃檹) that may be annoying to people wanting to use the local contexts: If we pass a block to
Honeybadger.context
I would expect the return value to be the result of the block, rather than theHoneybadger::Agent
instance.Imagine a case we have a critical payments-related process in our application and we want to make sure that all errors triggered in this process are tagged. Pseudocode:
With the current implementation we need to either create additional variable before the block to make sure the return value from
#call
remains the same:or we have to use
return
explicitly:While it still works, it feels unexpected.
I'm not sure if there's any actual API level expectation thatI just noticed that the reason it returns Agent instance is to allow chaining, e.g.Honeybadger.context
returnsAgent
instance (it's undocumented which is why I missed it) but I think it could make sense to modify the behaviour.Honeybadger.context.clear!
. I think it makes sense without the block but I would still modify the behaviour for the block version.Having the behaviour modified would also pair with
rescue
blocks well, e.g.Let me know what you think and I can prep the PR with a fix.
The text was updated successfully, but these errors were encountered: