Early client impl and docs scaffolding #6
Conversation
| return next | ||
|
|
||
|
|
||
| class OutboundInterceptor: |
There was a problem hiding this comment.
Maybe call this BaseOutboundInterceptor?
There was a problem hiding this comment.
I am not sure it has value anymore than a suggestion to call it BaseTemporalError would. While some Python classes do this, it's by no means universal (it's not BaseIntEnum or BaseTypedDict).
Also, I am intentionally allowing this to be instantiated directly (this will be especially true with classes temporalio.worker.WorkflowOutboundInterceptor) for the default implementation. I intentionally do not make these @abstractmethod or extend ABC for that reason.
There was a problem hiding this comment.
I'm fine with the naming, not sure why we need to allow direct instantiation though for the outbound interceptor though.
There was a problem hiding this comment.
Usually when subclassing you don't have to, but if people wanted to use composition instead they'd want to. Being able to make a "default interceptor" could have benefits in a ternary, e.g. interceptor = my_interceptor or OutboundInterceptor. Otherwise, it's just general decent practice to not force something to be abstract/base that has no implementation requirements.
There was a problem hiding this comment.
I don't see much value in having an empty interceptor in the chain TBH but with the inheritance based approach I can see why you'd want to allow this.
There was a problem hiding this comment.
I don't see much value in it either. I just don't like making something abstract unnecessarily. I would just as soon call this DefaultOutboundInterceptor.
| return next | ||
|
|
||
|
|
||
| class OutboundInterceptor: |
There was a problem hiding this comment.
I don't see much value in having an empty interceptor in the chain TBH but with the inheritance based approach I can see why you'd want to allow this.
| pass | ||
|
|
||
|
|
||
| class FailureError(TemporalError): |
There was a problem hiding this comment.
I'm fine with FailureError or TemporalFailureError. Prefer them over ServiceError because of the ApplicationError dissonance.
There was a problem hiding this comment.
Yeah that's fair. TemporalFailureError is a bit of a mouthful but maybe the most understandable
There was a problem hiding this comment.
It's in the temporalio namespace and I prefer not to stutter (people more often qualify their references in Python than Typescript). I only prefixed Temporal as the root error.
|
Enough consensus reached, merging. |
What was changed