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

New Spezi Standard Implementation #9

Merged
merged 17 commits into from
Aug 9, 2023
Merged

New Spezi Standard Implementation #9

merged 17 commits into from
Aug 9, 2023

Conversation

niallkehoe
Copy link
Contributor

@niallkehoe niallkehoe commented Aug 1, 2023

New Spezi Standard Implementation

♻️ Current situation & Problem

Currently, the SpeziHealthKit module lacks support for the new Spezi Standard released in Spezi 0.7.0
*This PR implements the improvements mentioned in [https://github.com/https://github.com/StanfordSpezi/Spezi/pull/73](Pull 73) *

💡 Proposed solution

Update the codebase to the newest Spezi 0.7.0

⚙️ Release Notes

This feature will allow for greater flexibility when using Spezi Questionnaire.

/// A Constraint which all `Standard` instances must conform to when using the Spezi HealthKit module.
public protocol QuestionnaireConstraint: Standard {
    /// Adds a new `HKSample` to the ``HealthKit`` module
    /// - Parameter response: The `HKSample` that should be added.
    func add(_ response: HKSample) async
    
    /// Removes a `HKSampleRemovalContext` from the ``HealthKit`` module
    /// - Parameter response: The `HKSampleRemovalContext` that contains information on the item that should be removed.
    func remove(removalContext: HKSampleRemovalContext)
}

Related PRs

See [https://github.com/https://github.com/StanfordSpezi/Spezi/pull/73](Pull 73) in Spezi for details

Testing

Passes all tests

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

@niallkehoe Thank you for the PR and the improvements to the HealthKit module! This really demonstrates how the new Standard implementation improves these types of interactions and how this leads to a cleaner UI for the Standards.

I had several comments across the PR, let me know if you have any questions or comments regarding the different elements! Happy to go into detail if you have any follow-up questions.

Sources/SpeziHealthKit/HealthKitConstraint.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/HealthKitConstraint.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/HealthKitConstraint.swift Outdated Show resolved Hide resolved
Tests/UITests/TestApp/TestApp.swift Outdated Show resolved Hide resolved
Tests/UITests/TestApp/HealthKitTestsView.swift Outdated Show resolved Hide resolved
Tests/UITests/TestApp/HealthKitTestsView.swift Outdated Show resolved Hide resolved
Tests/UITests/TestApp/ExampleStandard.swift Outdated Show resolved Hide resolved
Tests/UITests/TestApp/TestApp.swift Outdated Show resolved Hide resolved
Tests/UITests/TestApp/HealthKitTestsView.swift Outdated Show resolved Hide resolved
@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Aug 6, 2023
@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #9 (11cf6f5) into main (0a26f41) will increase coverage by 64.23%.
The diff coverage is 86.21%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##            main       #9       +/-   ##
==========================================
+ Coverage   7.56%   71.79%   +64.23%     
==========================================
  Files         12       11        -1     
  Lines        463      436       -27     
==========================================
+ Hits          35      313      +278     
+ Misses       428      123      -305     
Files Changed Coverage Δ
...althKit Extensions/HKHealthStore+SampleQuery.swift 0.00% <0.00%> (ø)
...Extensions/HKHealthStore+AnchoredObjectQuery.swift 85.19% <87.50%> (+85.19%) ⬆️
...hKit/CollectSample/HealthKitSampleDataSource.swift 96.33% <92.86%> (+96.33%) ⬆️
...s/SpeziHealthKit/CollectSample/CollectSample.swift 100.00% <100.00%> (+100.00%) ⬆️
.../SpeziHealthKit/CollectSample/CollectSamples.swift 100.00% <100.00%> (+70.59%) ⬆️
Sources/SpeziHealthKit/HealthKit.swift 88.41% <100.00%> (+50.38%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a26f41...11cf6f5. Read the comment docs.

@PSchmiedmayer
Copy link
Member

@niallkehoe Thank you for continuing to work on the PR, let me know once you have addressed all comments and you think that it is in a good shape to give it an other review 🚀

@niallkehoe
Copy link
Contributor Author

@paul Just uploaded my code after a check of all the files

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you for the improvements @niallkehoe! I only had a few very small comments that would be great to be addressed before we merge the PR. No need for an other review, you can merge the PR one the conversations are resolved.

Thank you for all the work and effort that went into improving Spezi and the HealthKit module!

Sources/SpeziHealthKit/HealthKitConstraint.swift Outdated Show resolved Hide resolved
Tests/UITests/UITests.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
Tests/UITests/UITests.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
Tests/UITests/UITests.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
Tests/UITests/TestApp/ExampleStandard.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/HealthKitConstraint.swift Outdated Show resolved Hide resolved
niallkehoe and others added 4 commits August 9, 2023 14:44
Co-authored-by: Paul Schmiedmayer <PSchmiedmayer@users.noreply.github.com>
Co-authored-by: Paul Schmiedmayer <PSchmiedmayer@users.noreply.github.com>
@PSchmiedmayer PSchmiedmayer merged commit 36a9d3b into main Aug 9, 2023
@PSchmiedmayer PSchmiedmayer deleted the newSpeziStandard branch August 9, 2023 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants