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

[BACK-2520] Adding sleep schedules to pump settings #78

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gniezen
Copy link
Member

@gniezen gniezen commented Jun 26, 2023

Adds the data model changes for BACK-2520. This is my first time making any changes to TidepoolApi, and my first time using OpenAPI, so please double-check that I've crossed all the t's and dotted all the i's.

I've also changed the location parameter for npm install to -g, as --location=global wasn't working for some reason.

@gniezen gniezen requested a review from tjotala June 26, 2023 09:28
16. [Units (`units`)](#units-units)
17. [Examples](#examples)
18. [Keep Reading](#keep-reading)
1. [Type (`type`)](#type-type)
Copy link
Member

Choose a reason for hiding this comment

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

I use VS Code with Markdown All In One which auto-generates TOCs :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It annoyed me that this ordered list would have to be updated every time you wanted to add a new item anywhere, so I changed it to all 1.s.

And then I just learned that Markdown doesn't care at all what numbers you use, as long as the first one is 1: https://www.markdownguide.org/basic-syntax/#ordered-lists

sleepSchedules:
type: array
minItems: 0
maxItems: 10
Copy link
Member

Choose a reason for hiding this comment

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

Is this imposed from some external spec, or just a reasonable upper limit?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a reasonable upper limit.

maximum: 86400
description: end time in seconds, offset from midnight (e.g. 7am is 25200)
required:
- enabled
Copy link
Member

Choose a reason for hiding this comment

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

Nit: extra indent, though I don't think YAML / OAS3 cares.

Copy link
Member

Choose a reason for hiding this comment

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

Is it fair to assume that if enabled: true then other fields are required?

Unfortunately I think the only way to express conditional requirement in OAS3 is rather ugly: defining two separate yet somewhat redundant types, one which requires all fields (with enabled: true), and the other with just enabled: false. And then stitch them together with a oneOf at the top level.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I tried that and it was so ugly that I took it out again 😂 The cleanest way I found was to use conditional operators (if/then/else) but that was only added in OAS 3.1, which doesn't seem to be fully supported by Stoplight. Redoc on the other hand: Redocly/redoc#1939

@tjotala
Copy link
Member

tjotala commented Jun 26, 2023

Adds the data model changes for BACK-2520. This is my first time making any changes to TidepoolApi, and my first time using OpenAPI, so please double-check that I've crossed all the t's and dotted all the i's.

I've also changed the location parameter for npm install to -g, as --location=global wasn't working for some reason.

Interesting. You might have an old npm version? That said, apparently the -g and --global switches were undeprecated (???) in 8.12, I'm guessing it was causing too much heartburn: https://github.com/npm/cli/releases/tag/v8.12.1

format: integer
minimum: 0
maximum: 86400
description: end time in seconds, offset from midnight (e.g. 7am is 25200)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for end <= start? AFAIK there's no cross-field validation in OAS3 so it's mainly idle curiosity.

Or perhaps more importantly, how does one express something like "start at 11pm on Tuesday, end at 7am Wednesday"? Something like { enabled: true, days: [ "Tuesday", "Wednesday" ], start: 8200, end: 25200 }? But that might be ambiguous?

Copy link
Member Author

@gniezen gniezen Jun 27, 2023

Choose a reason for hiding this comment

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

Yes, end can be smaller than start. In fact, that is the most likely case. To express your example, you would just do:

{ "enabled": true, "days": [ "Tuesday" ], "start": 82800, "end": 25200 }

@gniezen gniezen requested a review from tjotala July 4, 2023 15:55
@gniezen
Copy link
Member Author

gniezen commented Jul 31, 2023

@tjotala Anything else that you need me to do on this PR?

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