Closed subzero10 closed 1 year ago
Hey @shalvah, I am working on the final part of this new checkins feature, which is to allow the user to checkin using the name instead of the id.
For example, a checkin "A simple checkin" could be called with $honeybadger->checkin("A simple checkin")
. This is the final change to allow users to configure checkin definitions entirely from the config file and not having to look for information in the dashboard.
I'm facing a small problem and I'm experimenting on what's the best way to tackle the following:
CheckinsClient.php
class throws ServiceException
s and does not handle them.HoneybadgerClient.php
class allows ServiceException
s to be handled through a configuration parameter.Honeybadger.php
class uses HoneybadgerClient.php
to reach the API. One of the API calls is checkin($id)
.CheckinsClient.php
(through Honeybadger.php
) to call the APP (not API - that's why we're using a personal auth token) in order to get information about a project's checkins and find the id of the checkin I'm looking for.CheckinsClient.php
and use the configurable handler to catch all thrown exceptions. One idea I came up with was to use add a boolean flag in the CheckinsClient.php
class ($useExceptionHandler
), wrap each method with a try/catch block and decide whether to throw or call the handleServiceException
depending on the value of $useExceptionHandler. However, I found this to be too verbose and noisy.__call
magic method that wraps the call to the real class with a try catch block. But I don't know if this is good practice in the PHP world.handleServiceException
method from ApiClient.php
in Honeybadger.php
and wrap all the calls $checkinsClient
with a try/catch block.Do you have any thoughts on this? How would you go about it?
Honestly...I don't know. I wouldn't say the proxy class is a bad practice, or even discouraged, especially as you're using it for an internal detail. Other alternatives would be inheritance or implementing an interface, but I think this is fine (and I think it's a form of composition), at least for the current use case. 👍 I would recommend naming it more specifically, though e.g. CheckInClientWithErrorHandling
.
I'll go ahead and merge, considering @shalvah's feedback above. I applied the suggested change 👌.
Status
READY
Description
The last piece of the next iteration of Checkins. This change will allow users to use checkin names instead of ids. Closes #175.
Related PRs
Todos