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.

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

Project APIs #40

Merged
merged 8 commits into from
Sep 1, 2022
Merged

Project APIs #40

merged 8 commits into from
Sep 1, 2022

Conversation

mmertsock
Copy link
Member

@mmertsock mmertsock commented Aug 11, 2022

Overview

Implements model types, API calls, example app views, and unit tests for project info APIs.

See https://developer.mydatahelps.org/sdk/project_settings.html to compare to JavaScript SDK counterparts.

Security

REMINDER: All file contents are public.

  • I have ensured no secure credentials or sensitive information remain in code, metadata, comments, etc. Of particular note:
    • No temporary testing changes committed such as API base URLs, access tokens, print/log statements, etc.
    • Xcode project/target settings should remain generic. Don't leak team identifiers, code signing info, local filesystem paths, etc.
  • My changes do not introduce any security risks, or any such risks have been properly mitigated.

Describe briefly what security risks you considered, why they don't apply, or how they've been mitigated.

Checklist

  • All public symbols are documented using Swift Markup comments.
  • If this feature requires a developer doc update, tag @CareEvolution/api-docs.
  • Source code file header comments are clean and standardized. Use "Created by CareEvolution on m/dd/yy".
  • Test and update the example app as needed. The example app should demonstrate all features of the SDK, and it's the most convenient way to test your feature.

Consider "Squash and merge" as needed to keep the commit history reasonable on main.

@mmertsock mmertsock force-pushed the mmertsock/project-apis branch from 06de281 to cb98d99 Compare August 11, 2022 18:50
@mmertsock mmertsock mentioned this pull request Aug 11, 2022
@mmertsock mmertsock requested a review from eschramm August 26, 2022 18:26
Copy link

@eschramm eschramm left a comment

Choose a reason for hiding this comment

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

LGTM - just a question about ProjectType


/// Describes the type of a MyDataHelps project.
/// May be one of: Research Study, Wellness Program, or Clinical Program.
public struct ProjectType: RawRepresentable, Equatable, Decodable {

Choose a reason for hiding this comment

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

Any reason why this would be a struct instead of an enum?

Copy link
Member Author

Choose a reason for hiding this comment

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

For forward compatibility. If this were struct ProjectType: String, and we introduce a new ProjectType value on the server, then the SDK would immediately fail to decode the ProjectInfo for such projects, throwing a decoding error. Using a RawRepresentable struct instead, these new ProjectType values successfully decode, simply as instances with different rawValues.

This pattern is used with most of the enum-like decodable values in the SDK. In practice, the syntax for using these structs (choosing specific values or checking for equality to specific values) looks identical to plain enums, as seen throughout the example app. e.g. you can just check for projectInfo.type == .researchStudy, use the shorthand dot-syntax in switch cases, etc.

See https://github.com/CareEvolution/MyDataHelpsKit-dev/pull/1#discussion_r598414777 for the original discussion about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, your comment has brought attention to the unnecessary documentation comment "May be one of…", which I copied/pasted from the JS SDK. I'll remove that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated unit tests to verify/clarify handling of unknown "enum" values.

@mmertsock mmertsock force-pushed the mmertsock/project-apis branch from 7dbcb60 to 08786dd Compare August 31, 2022 14:02
Copy link

@eschramm eschramm left a comment

Choose a reason for hiding this comment

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

Changes LGTM 👍

@mmertsock mmertsock merged commit 5492975 into main Sep 1, 2022
@mmertsock mmertsock deleted the mmertsock/project-apis branch September 1, 2022 14:53
This was referenced Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants