-
Notifications
You must be signed in to change notification settings - Fork 98
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
External User IDs Improvements #1092
Conversation
return StorageUtils.getExternalUserIds() | ||
/// This method allows to get all External User Ids | ||
@available(*, deprecated, message: "Deprecated. SDK doesn't support storing External User IDs in application storage. This method will be removed in future releases.") | ||
public func fetchStoredExternalUserIds() -> [ExternalUserId]? { |
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.
The method storeExternalUserId
should be also deprecated.
arrayExternalUserIds.removeAll() | ||
StorageUtils.setExternalUserIds(value: arrayExternalUserIds) | ||
} | ||
externalUserIds = [] | ||
} | ||
|
||
public func getExternalUserIds() -> [[AnyHashable: Any]]? { |
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.
There are a lot of changes in the PR, so I can miss something, but now I see incosistency in how external ids can be set and retirived.
I see two public items:
- the property
externalUserIds
- the method
getExternalUserIds
And the method has broader functionality. To make everything consistent, I'd suggest making externalUserIds
private and to add the setExternalUserIds
method. Or you can introduce a custom getter for externalUserIds.
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
@OlenaPostindustria, please, rebase the branch. |
bdd0fab
to
8392cb5
Compare
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
Closes #1091