-
Notifications
You must be signed in to change notification settings - Fork 120
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 cancellation token parameter to async methods #549
Conversation
I think for some of those methods that take parameterized array such as: we can probably leave the old definition as is i.e.: and add another one that takes the token (not optional but required) while remove this signature that I modified it to earlier: This way it doesn't break other codebases. Although its best practice to take token as the last default parameter but not sure how best to get away with this. |
@matt-richardson any thoughts on above? and when can this be merged and available for usage once ready. Appreciate any feedback! |
Hi @yj7o5 - thanks for the PR. I've passed it onto the right team here to look at. Hopefully they should get to it soon. As for the param ordering, we could either put the cancellation token first, or add an overload that drops the |
Unfortunately the QRF team did not have capacity to address this PR. We'll keep the PR open, but unfortunately we can not provide any guidance as to when it will be resolved. |
That's fine. Appreciate the response. |
CancellationToken
to all the async public methodsFew notes:
I have removed one of the PUT definition defined under the
IOctopusAsyncClient
because after adding the optionalCancellationToken
it resulted in ambiguous calls betweenPUT(string path, TResource resourse, CancellationToken token = default))
andPUT(string path, TResource resourse, object pathParameters = null, CancellationToken token = default))
. Therefore removed the former one and keeping the later. Everything should work just fine for PUTs in IOctopusAsyncClient.86e2f6b#diff-2c952992d6d1233543cc4f2e60805c56L248
The GETs here are taking CancellationToken as first parameter rather as the last being last in the list because of the
params
parameter array limitation. Not really sure on how to go about this one, maybe we don't support CancellationToken for these GET calls :/ ? ... Because this might potentially break public code, since it requires the calls to be made as named parameter unless users pass in the cancellation token as the first paramter:https://github.com/OctopusDeploy/OctopusClients/pull/549/files#diff-847456346808210c60e2f723afc5ebedR32
https://github.com/OctopusDeploy/OctopusClients/pull/549/files#diff-6afea74a6e0295e940e3811d8b8a565aR232
https://github.com/OctopusDeploy/OctopusClients/pull/549/files#diff-4c263f6d32ae87b4ba88412ad4a3e175R11
closes #537