-
Notifications
You must be signed in to change notification settings - Fork 34
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
Feature/add cancellation tokens #285
Conversation
@@ -76,6 +76,7 @@ public async Task<AvatarCreationResponse> CreateAvatarAsync(AvatarProperties ava | |||
} | |||
catch (Exception e) | |||
{ | |||
ThrowErrIfCancellationException(e); |
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.
It's super annoying that we have to add this in all the exception parts of this script.
It could be moved into some sort of "HandleException(ex)" function to reduce the code duplication.
Eg
private void HandleException(Exception e)
{
ThrowErrIfCancellationException(e);
OnError?.Invoke(e.Message);
}
It might also be nice in future to change the OnError action to have a parameter of type Exception. Then we can just use that for everthing (including throwing the cancellation exception). It would be a breaking change though.
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.
Yep, can move onError there as well.
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.
You can actually use the built in exclude logic to handle this, if you wanted a more graceful solution ->
catch (Exception ex) when (ex is not OperationCanceledException)
https://stackoverflow.com/questions/12633903/elegantly-handle-task-cancellation
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.
Runtime/AvatarCreator/Scripts/UI/Elements/DeleteAvatarElement.cs
Outdated
Show resolved
Hide resolved
Runtime/AvatarCreator/Scripts/UI/Elements/DeleteAvatarElement.cs
Outdated
Show resolved
Hide resolved
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.
Great work on this btw
SDK-971
Description