-
Notifications
You must be signed in to change notification settings - Fork 18
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: Auth Ready, resolves IBM/unity-sdk-core#32 #33
base: main
Are you sure you want to change the base?
Conversation
…while (!auth.CanAuthenticate)
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.
Left one question, overall it does look fine. I will take some time out to do some testing before approving today. Once again thanks a lot for the PR.
@@ -131,6 +134,7 @@ private void GetToken() | |||
|
|||
private void OnGetToken(DetailedResponse<CloudPakForDataTokenResponse> response, IBMError error) | |||
{ | |||
isDoneInitializing = true; |
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 isDoneInitializing
be called after we throw the error?
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, yes.
If we move the new line a few lines down (inside else
statement) so it's not called when an error happens, then there would be no way left for a consumer class to know if an error happened.
If you think this is not clear enough, maybe we should have an IsWaiting
field instead, it would have the opposite value to IsDoneInitializing
, and maybe makes thing more clear?
Added
Authenticator.IsDoneInitializing
field socan become: