Skip to content

Conversation

@ps2
Copy link
Owner

@ps2 ps2 commented Mar 5, 2019

No description provided.

@ps2 ps2 merged commit fa8f601 into dev Mar 5, 2019
Copy link
Contributor

@mpangburn mpangburn left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@@ -1 +1 @@
github "LoopKit/LoopKit" "dev"
github "tidepool-org/LoopKit" "basal-picker"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's your typical Cartfile strategy for merging changes that depend on different branches? Switch to the dependent branch while testing, then revert the Cartfile after merging?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. For me, I can build in a workspace setup, and not worry to much about updating carthage specs, but travis needs it updated, and if others check out the branch, it helps them be able to build.

@ps2 ps2 deleted the basal-picker branch March 5, 2019 20:04
Copy link
Contributor

@darinkrauss darinkrauss left a comment

Choose a reason for hiding this comment

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

I think the range for rateGroup1 is incorrect and the Cartfile and Cartfile.resolved need to be fixed to point back to LoopKit/LoopKit, but otherwise looks good.

public var supportedBasalRates: [Double] {
if generation >= 23 {
// 0.025 units (for rates between 0.0-0.975 U/h)
let rateGroup1 = ((0...38).map { Double($0) / Double(pulsesPerUnit) })
Copy link
Contributor

Choose a reason for hiding this comment

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

Should 38 be 39?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ps2 Here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks. Fixed here: 23af716

}

public var maximumBasalScheduleEntryCount: Int {
return 48
Copy link
Contributor

Choose a reason for hiding this comment

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

Really? Wow! I didn't know it supported 48.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, It was a little bit of a pain to test, but the pump accepted it!

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.

4 participants