- Notifications
You must be signed in to change notification settings - Fork 298
Refactoring the User Management API #37
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
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.
Basically LGTM but a few minor issues.
| * corresponding to the newly created account. If an error occurs while creating the user | ||
| * account, the task fails with a FirebaseAuthException. | ||
| * @throws NullPointerException if the provided builder is null. | ||
| * @throws NullPointerException if the provided user is null. |
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.
user => request ?
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.
Done
| * | ||
| * @return a user ID string. | ||
| */ | ||
| public String getUid() { |
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 these be annotated with @ Override?
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.
Done
| * @return a display name string or null. | ||
| */ | ||
| @Nullable | ||
| public String getDisplayName() { |
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.
Not sure, but I think we still want @ Nullable annotation?
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.
Done
| | ||
| /** | ||
| * Returns a new {@link User.Updater} instance, which can be used to update the attributes | ||
| * Returns a new {@link UpdateRequest}, which can be used to getProperties the attributes |
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.
"used to getProperties" ?
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.
silly auto refactor! Fixed :)
| Made the suggested changes. |
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!
| +1 |
Following changes were suggested at the API review:
UserInfopublic interface typeUsertoUserRecordUser.BuildertoUserRecord.CreateRequestUser.UpdatertoUserRecord.UpdateRequestProviderUserInfofrom the public APIUserInfoat bothUserRecordandProviderUserInfoRelated to #33