Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add user lookup API #1486

Merged
merged 6 commits into from
Jul 24, 2017
Merged

Add user lookup API #1486

merged 6 commits into from
Jul 24, 2017

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Jul 19, 2017 •
edited
Loading

Description

Fixes #1457

TODO

  • Changelog entry
  • Tests (if applicable)
  • Update PCL (if applicable)

@nirinchev nirinchev requested a review from kristiandupont July 20, 2017 09:45
kristiandupont
kristiandupont approved these changes Jul 20, 2017
View reviewed changes
Platform.PCL/Realm.Sync.PCL/UserPCL.cs Outdated
/// e.g. on Facebook and want to find the associated Realm user's Id.
/// </summary>
/// <param name="provider">The provider that the user has signed up with.</param>
/// <param name="providerId">The id of the user in the provider's system.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate this name. Is it publicly exposed in the other SDK's?

Copy link
Member Author

Choose a reason for hiding this comment

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

In js it's provider, username, in Java, it's provider, providerId.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm struggling to find a better name, but I'm definitely open for suggestions!

@bdash
Copy link
Contributor

bdash commented Jul 20, 2017

Did y'all take a look at what the Java and JavaScript versions of these APIs are like?

@nirinchev
Copy link
Member Author

nirinchev commented Jul 20, 2017 •
edited
Loading

I did, why? Javascript seems to be returning the json as is from the server, while Java is parsing that into a SyncUser instance. Do you have a strong preference toward either of the two approaches? The one I chose is close to js's.

fealebenpae
fealebenpae reviewed Jul 24, 2017
View reviewed changes
Realm/Realm.Sync/Dtos/UserInfo.cs Outdated
@@ -21,7 +21,7 @@ namespace Realms.Sync
/// <summary>
/// The response of a user lookup operation.
Copy link
Member

Choose a reason for hiding this comment

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

The comment could use updating now that the class is renamed.

@nirinchev
Copy link
Member Author

CI failures will be resolved by #1489.

@nirinchev nirinchev merged commit b428a2f into master Jul 24, 2017
@nirinchev nirinchev deleted the ni/user-lookup branch July 24, 2017 15:29
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fealebenpae fealebenpae fealebenpae left review comments

@kristiandupont kristiandupont kristiandupont approved these changes

Assignees

@nirinchev nirinchev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nirinchev @bdash @fealebenpae @kristiandupont