fix(dispatch): Dispatch for transform. Transformer instead of Service #1691
Conversation
| } | ||
|
|
||
| // CoreRequestsName implements the Requests interface | ||
| func (TransformMethods) CoreRequestsName() string { return "apply" } | ||
| // Name returns the name of this method gropu |
transform/apply.go
Outdated
| // Service applies transform scripts to datasets | ||
| type Service struct { | ||
| bgCtx context.Context | ||
| // Transformer holds long-lived values needed to apply transforms |
There was a problem hiding this comment.
Transformer to my go brain should be a single-method interface:
type Transformer interface {
Transform()
}I can think of two plausible alternatives:
- Write an
Applicatorinterface with theApplyfunction as it's only method & flip the currenttransformerto a private implementation - Just call this
transform.Context, because that's what it is.
I'm a fan of the second, personally. I know context is a thing, and is used here, but for external callers it'll always be transform.NewContext returning a transform.Context.
As for the comment, "long-lived" values seems a little vague, and not all of these refer to time how about:
Transformer holds values that span the application of a transform script
There was a problem hiding this comment.
I'm fine picking a better name, and certainly improving the comment, but just want to go through the thought process in detail.
The way I'm thinking about this, is that the values living on the Transformer struct are references to objects which are owned by other subsystems, and should not need to be parameterized by each call to Apply. That is, it should be possible for some user of the transform package to construct a single Transformer and then call Apply say 3 times (obviously lib is not going to do this, but by setting it up this way, the code naturally becomes less coupled and more modular). In doing so, it's highly unlikely they would need a different application context or event publisher or resolve function for each call, but they would certainly be using a different runID, secrets, probably dataset, etc. Another way to state this is that transform.Apply has both dependencies and inputs. The dependencies live on the struct, while the inputs are passed into Apply as arguments.
This is what leads me to believe that using a struct that only wraps the application context isn't going far enough. That seems to be done only the avoid going with a non-method Apply that needs to be passed in two context values (which would be incredibly bad!). With this change, I think it's worth considering the UX for a non-lib user. Most especially, I do not think it should be turned into transform.NewContext as that would be misleading: the context in question is not new, it's always going to be the top-level application context that was constructed at the process's start.
Also, in general, I don't like introducing interfaces that neither invert dependencies nor introduce polymorphism. That always feels like the interface is being added too early, for it's own sake, which adds friction without any gain. But I hear what you're saying about the name following go's "verb"er pattern which can be confusing. How about Transformation instead?
There was a problem hiding this comment.
Oh, one other thing I forgot to mention: justification for why this Transformation (or TransformService as it was also called) should not live on lib.Instance.
While the dependencies also live on lib, we want to be able to let the scope have control over those dependencies. For example, if we change event.Bus such that it only receives events scoped to the current user, that means we need to retrieve the bus from the scope. Hence, we don't want the Instance to keep control of the Transformation and hand it out as needed, rather we should be constructing it just before the Apply callsites.
There was a problem hiding this comment.
ah sorry, I didn't add enough detail in my first comment, but I'm glad you took the time to lay it out because I completely agree, and could see where you're going with this from the code as-presented (a good thing).
In saying "it's a context", I meant in the abstract sense of a context, which I think is what you're describing here. It should hold a set of scoped values for n number of related calls to Apply. I'm all for the fields you've added here.
But with that said, I tried to improve on terminology by suggesting we call it context, and clearly it didn't work 😄! So while I'm not a fan of the "verb"er overlap, it seems like the least offensive option. I'm not a fan of Transformation because, as you've pointed out, we want to closer to a factory-function-type pattern that encourages the multiple calls to apply mental model. Transformation implies a single call.
So, with that, I vote we change nothing–erm–I mean we keep this change as proposed? whatever, I agree it should be called Transformer 😄
There was a problem hiding this comment.
Sure thing, glad we're pretty much on the same page!
| if svc == nil { | ||
| return fmt.Errorf("transform service does not exist") | ||
| } |
There was a problem hiding this comment.
Having the method directly on the value makes this nil check safer to remove
There was a problem hiding this comment.
I'm convinced, no need for the receiver to use a pointer.
transform/apply.go
Outdated
| type Transformer struct { | ||
| AppCtx context.Context | ||
| LoadFunc dsref.ParseResolveLoad | ||
| Pub event.Publisher |
There was a problem hiding this comment.
Considering we're using a constructor, I'd vote to keep the fields of Transformer unexported to force use of NewTransformer
transform/apply.go
Outdated
| func NewTransformer(appCtx context.Context, loadFunc dsref.ParseResolveLoad, pub event.Publisher) *Transformer { | ||
| return &Transformer{ | ||
| AppCtx: appCtx, | ||
| LoadFunc: loadFunc, | ||
| Pub: pub, | ||
| } | ||
| } | ||
|
|
||
| // Apply applies the transform script to a target dataset | ||
| func (svc *Service) Apply( | ||
| func (t *Transformer) Apply( |
There was a problem hiding this comment.
Drop the pointer receiver for Apply and return just Transformer from NewTransformer. Apply shouldn't be able to affect values on Transformer, especially across multiple calls
No description provided.