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

Advanced meal assist #125

Merged
merged 230 commits into from
Jul 12, 2016
Merged

Advanced meal assist #125

merged 230 commits into from
Jul 12, 2016

Conversation

bewest
Copy link
Member

@bewest bewest commented May 29, 2016

Dev pathway for ama happens here.

scottleibrand and others added 18 commits June 24, 2016 14:57
* A couple more temp basal IOB tests; lib/iob/history.js is now pretty well covered

* Configurable safety limits pass 1, with tests

* Added --update-preferences and --export-defaults options to oref0-get-profile
* Change from json -e to json -E for speedup

* Some more BG conversions in determine-basal output
Add model_data to profile, fix usage string
Merge dev into ama via merge-ama branch
@scottleibrand
Copy link
Contributor

Checks are now passing, so this is a candidate for merge if no one flags any issues. I believe @cjo20 said things looked good for him. Anything else we need to do before merging to dev?

@cjo20
Copy link
Contributor

cjo20 commented Jul 11, 2016

Yup, things looked good for me when I upgraded. I'm not sure if the reordering of the yargs stuff in oref0-get-profile has introduced any upgrade problems, but it should be minor

@scottleibrand
Copy link
Contributor

Yeah, I'm not too worried about anything that only affects people who've added a feature in the last week or two. :-)

@scottleibrand
Copy link
Contributor

One last call for any objections to merging AMA to dev.

@@ -105,7 +106,7 @@ if (!module.parent) {
console.error(msg.msg);
// console.log(JSON.stringify(msg));
if (!params['missing-meal-ok']) {
errors.push(msg);
warnings.push(msg);
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, --no-missing-meal-ok means "strict" mode which I would prefer throw an error (since it's being asked for).

Copy link
Contributor

Choose a reason for hiding this comment

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

If you'd like a flag for strict mode that's fine, but the default needs to be to warn, not break the loop. This has caused us issues several times due to network comms issues (like wifi captive portals) when configuration is fine.

Copy link
Member

Choose a reason for hiding this comment

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

for interactive/manual mode strict is nice, since it will help spot issues, but when running a loop it's a problem

I added --missing-meal-ok to my rigs to prevent issues, but I expect others to miss that and have their loop fail.

@bewest
Copy link
Member Author

bewest commented Jul 12, 2016

Left a few notes regarding tips on maintaining positional arguments vs option switches over the long haul. Overall I'm excited to get this into dev and subsequently do a release.

@scottleibrand
Copy link
Contributor

Sounds like we should merge to dev and then do some additional yargs conversion (in addition to the stuff that has recently been added) before releasing to master. If no objections, I'll do the merge to dev today.

@jasoncalabrese
Copy link
Member

I'd be ok requiring the --auto-sens, --meal, and other options, maybe the merge to dev is the time to do that, I updated my rigs to use the options and it's not a big deal.

@scottleibrand scottleibrand merged commit 2446f76 into dev Jul 12, 2016
@scottleibrand scottleibrand deleted the advanced-meal-assist branch July 12, 2016 23:19
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.

9 participants