-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update CredentialPack to use ParsedCredential #39
Update CredentialPack to use ParsedCredential #39
Conversation
} | ||
} | ||
|
||
/// Get a credential by id. | ||
public func get(credentialId: Uuid) -> ParsedCredential? { |
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.
I'm a bit weirded out by having two functions with the same name and just an argument name that differs by one letter, but that might also just be a Swift convention, and I don't know Swift that well.
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.
I think this is a fairly normal Swift convention, but maybe @todd-spruceid or @Juliano1612 can weigh in.
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.
Yeah, that's common. You only can't have multiple methods with the same signature.
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.
One small comment but LGTM.
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! Started this spruceid/mobile-sdk-ios-app#20 PR to make the demo app compatible with these changes
No description provided.