Skip to content

Conversation

@hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented Jun 2, 2017

Following changes were suggested at the API review:

  • Introduce UserInfo public interface type
  • Rename User to UserRecord
  • Rename User.Builder to UserRecord.CreateRequest
  • Rename User.Updater to UserRecord.UpdateRequest
  • Drop ProviderUserInfo from the public API
  • Implement UserInfo at both UserRecord and ProviderUserInfo

Related to #33

Copy link

@mikelehen mikelehen left a 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

user => request ?

Copy link
Contributor Author

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() {

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?

Copy link
Contributor Author

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() {

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?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"used to getProperties" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

silly auto refactor! Fixed :)

@hiranya911 hiranya911 assigned hiranya911 and unassigned mikelehen Jun 5, 2017
@hiranya911
Copy link
Contributor Author

Made the suggested changes.

@hiranya911 hiranya911 assigned mikelehen and unassigned hiranya911 Jun 5, 2017
Copy link

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hiranya911 hiranya911 assigned hiranya911 and unassigned mikelehen Jun 5, 2017
@hiranya911 hiranya911 merged commit bdc640b into master Jun 5, 2017
@hiranya911 hiranya911 deleted the hkj-user-api-change branch June 5, 2017 17:53
@Solido
Copy link

Solido commented Jun 5, 2017

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants