Make averageHeartRate optional
#16
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kvs-coder
reviewed
Jul 4, 2022
| // MARK: - CustomStringConvertible | ||
| @available(iOS 14.0, *) | ||
| extension HKElectrocardiogram.Classification: CustomStringConvertible { | ||
| public var key: String { |
Owner
There was a problem hiding this comment.
I would rather not introduce another field for the JSON payload, but change the values in description here
kvs-coder
reviewed
Jul 4, 2022
| // MARK: - CustomStringConvertible | ||
| @available(iOS 14.0, *) | ||
| extension HKElectrocardiogram.SymptomsStatus: CustomStringConvertible { | ||
| public var key: String { |
kvs-coder
reviewed
Jul 4, 2022
|
|
||
| let samplingFrequencyUnit = HKUnit.hertz() | ||
| guard | ||
| let samplingFrequency = samplingFrequency?.doubleValue(for: samplingFrequencyUnit) |
Owner
There was a problem hiding this comment.
it appears that the samplingFrequency can also be nullable, could please introduce the optionality here the same way you did for the averageHeartRate? For reference
kvs-coder
requested changes
Jul 4, 2022
Owner
There was a problem hiding this comment.
Hi @krml19
many thanks for your PR. I reviewed it at left some comments. I would introduce in the frame of this PR the sampleFrequency nullability as well and remove the key properties for Classification and SymptomsStatus you added. You can use description property and change the raw strings
kvs-coder
force-pushed
the
master
branch
7 times, most recently
from
ddb82fa to
7ee3437
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
READY
Migrations
NO
Description
There is no option to collect the voltage measurements for electrograms with inconclusive poor readings because they do not have any value for
averageHeartRate. Therefore, we should allow fornilvalue foraverageHeartRate.