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

Additional documents for CollectSample and HealthKitConstraint #18

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

YurenSUN
Copy link
Contributor

@YurenSUN YurenSUN commented Feb 28, 2024

Additional documents for CollectSample and HealthKitConstraint

♻️ Current situation & Problem

When working with this package for our PICS application, I found it a bit confusing about the add and remove functions in the HealthKitConstraint as I misunderstood them to be adding/removing health kit samples to/from the data sources instead of our application. Also, when using CollectSample, I spent some time finding how to filter the health kit query to collect data within the specific time range. I thought that this package did not support such filtering until I looked into the codes and realized that I could also input the predicate to it. I also found that the documentation mentioned the predicate initializations for CollectSample only after I found the predicate parameter in the codes, and would appreciate that if it could be mentioned more clearly.

I think adding more documentation and code examples for clarification will possibly not benefit those who are familiar with Swift documentation or HealthKit packages, but might help users who are new to Swift to get on board with this package more easily.

⚙️ Release Notes

  • Added a code example including predicates for CollectSample
  • Clarify the add and remove functions for HealthKitConstraint

📚 Documentation

Only additional documents were added were added in this PR.

✅ Testing

Only additional documents were added and no codes were changed. Also checked and verified the changes in the documentation by building documentation locally.

📝 Code of Conduct & Contributing Guidelines

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

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.90%. Comparing base (b40695f) to head (bb9fa32).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage   69.90%   69.90%           
=======================================
  Files          11       11           
  Lines         455      455           
=======================================
  Hits          318      318           
  Misses        137      137           
Files Coverage Δ
...s/SpeziHealthKit/CollectSample/CollectSample.swift 100.00% <ø> (ø)
Sources/SpeziHealthKit/HealthKit.swift 76.75% <ø> (ø)

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 b40695f...bb9fa32. Read the comment docs.

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 PR @YurenSUN; this will help future students and Spezi users a lot!

I had a few suggestions there and there about smaller improvements, feel free to re-request a review once these are resolved 🚀

README.md Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/HealthKitConstraint.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/HealthKit.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/CollectSample/CollectSample.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/CollectSample/CollectSample.swift Outdated Show resolved Hide resolved
Sources/SpeziHealthKit/CollectSample/CollectSample.swift Outdated Show resolved Hide resolved
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.

@YurenSUN Thank you for incorporating the improvements! I only had a small last comment; the PR already looks great!

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 and documentation additions! 🚀

@PSchmiedmayer PSchmiedmayer merged commit 4252621 into StanfordSpezi:main Mar 1, 2024
8 checks passed
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