Closed jhiker closed 1 year ago
Thanks for implementing these hooks @jhiker ! Looks good to me.
A few additional callouts for future work:
- I think we've got a lot going on in the
Client
class now. I'd be game for moving that builder logic into a separate class when we implement the next type of hook.- We'll want to update the README to demonstrate how one can use before hooks
- I still wonder if having to subclass
BeforeHook
is going to be cumbersome for users of the gem, especially if you're trying to make an ad-hoc hook to pass in during invocation. I think sub-classing will make a lot of sense for Providers and plugins like Datadog to ship strongly-typed hooks though!
1 and 2 makes sense. on 3, we could change the Hook subtyping to use composition if we think that is more ergonomic, bearing in mind the caveat I mentioned earlier. i'm still partial to the current approach for that reason, but perhaps we wait and see?
we also should add in support for after, error, and finally hooks. those should be fairly trivial to add in comparison as they just side-effect. also add in hook hints, whatever those are used for, when that makes sense. THEN WE ARE DONE WITH HOOKS
thanks for taking this initiative and giving me the opportunity to play with Sorbet.
This introduces support for before hooks as per specification. I deliberately left out hints as it is pretty complex already.
What is deliberately left out: 1) Passing "hook hints" all the way through properly 2) There are some conditions that are not tested, although they are mostly trivial (e.g. sequencing of hooks) 3) The other types of hooks. These other ones should be pretty easy to add in a follow-up: we just create more sub-types and insert in the right place, and these other types don't have the spec that the evaluation context has to be passed through but instead just side-effect