-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add new TaskOption overload for skipping input param #110
Conversation
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.
Almost ready to approve, just a minor question and nitpick
|
||
/// <returns> | ||
/// A task that completes when the activity completes or fails. The result of the task is the activity's return value. | ||
/// </returns> | ||
/// <inheritdoc cref="CallActivityAsync(TaskName, object?, TaskOptions?)"/> | ||
public virtual Task CallActivityAsync(TaskName name, TaskOptions options) | ||
=> this.CallActivityAsync(name, null, options); |
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.
Should we really inherit the docs from the CallActivityAsync
with the input parameter?
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.
Same comment for the changes in CallSubOrchestrator
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.
To be honest, I don't really know the answer to that. @cgillum suggested I do it on another PR so I did 🤷
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.
Well, even if this were off, it's a non-blocking concern
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.
LGTM, although it would be good to hear from @cgillum on the inheritdoc
question
I will address/review the inheritdoc usages in a later PR. |
Adds some overloads to protect against accidentally supplying
TaskOptions
asinput
.