-
Notifications
You must be signed in to change notification settings - Fork 48
Initial implementation of plugins #548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cretz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me, but I'm a bit biased because I was involved in the coding. @maciejdudko - can you take a look at this review?
jmaeagle99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrote some thoughts down, but feel free to take or leave any of them.
| /// <summary> | ||
| /// Gets the constant value for the required option. | ||
| /// </summary> | ||
| public T? Constant { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this nullable? Would it not be sufficient to declare the usage of the SimplyPluginOption<T> as nullable itself? The only scenario I can think of where you might need this is if you want to say that this option doesn't have a non-null default value and you want to have the configurable callback; this feels like a simplified design of the options pattern in .NET where any number of consumers of the library could implement IConfigureOptions<T> and IPostConfigureOptions<T> for declaring and registering callbacks for mutating options before a consume of the options gets the final value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does sound similar. It's trying to mimic what I did in the other languages with either taking a value or a function that produces it. The options pattern seems to be doing the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this side of the SDK, we don't make any assumptions about IConfigureOptions and such, so we can only provide naive forms of callback-based options mutations. So this is effectively a "constant or function" mutually exclusive option value. Technically we could have made a whole abstract base class sum type thing, but this class should be rarely used anyways.
What was changed
Added
ITemporalClientPluginandITemporalWorkerPluginto provide configurability to their respective objects.SimplePluginwas added to make it easier to implement plugins consistently across objects.Why?
It can be difficult to provide consistent and easy methods of configuring temporal objects.
Checklist
Closes [Feature Request] Plugin support #507 #plu
How was this tested:
New set of plugin tests.
Any docs updates needed?
Plugin documentation in general is pending.