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.

Sign up for GitHub

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

Jump to bottom

removed Optional on the specified datakey fields #313

Merged
merged 3 commits into from
Sep 23, 2024
Merged

removed Optional on the specified datakey fields #313

merged 3 commits into from
Sep 23, 2024

Conversation

evalott100
Copy link
Contributor

@evalott100 evalott100 commented Sep 17, 2024 •
edited by coretl
Loading

Fixes half of #309

@evalott100 evalott100 self-assigned this Sep 17, 2024
@evalott100 evalott100 force-pushed the 309-add-not-required-things-to-datakey-schema branch from 194e6b4 to 1e3f44a Compare September 17, 2024 08:43
coretl
coretl approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@coretl coretl left a comment

Choose a reason for hiding this comment

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

#309 isn't a very well written ticket... This PR does half of it.

Please could you make another PR that adds limits and choices as per the link in the ticket and make them NotRequired fields.

DiamondJoseph
DiamondJoseph reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@DiamondJoseph DiamondJoseph left a comment

Choose a reason for hiding this comment

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

Aren't units and precision optional though?
For a str enum precision doesn't make sense- are we just going to say 0 or -1? Empty units for dimensionless things yes

@coretl
Copy link
Contributor

coretl commented Sep 17, 2024

Aren't units and precision optional though? For a str enum precision doesn't make sense- are we just going to say 0 or -1? Empty units for dimensionless things yes

We remove the ability to make them null. This means they don't need to be in the descriptor (e.g. precision for an enum), but if they are present they must be of the right type.

DiamondJoseph
DiamondJoseph approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@DiamondJoseph DiamondJoseph left a comment

Choose a reason for hiding this comment

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

That makes sense: precision is still NotRequired but if present is str not str | None

@evalott100
Copy link
Contributor Author

Please could you make another PR that adds limits and choices as per the link in the ticket and make them NotRequired fields.

May as well do it here

This was linked to issues Sep 18, 2024
Add NotRequired things to the DataKey schema #309
Closed
Please export Dtype in event_model.__all__ #314
Closed
@evalott100
added epics limits to DataKey
72e40cd 
Also made `Dtype` accessible from `event_model`
@evalott100 evalott100 force-pushed the 309-add-not-required-things-to-datakey-schema branch from 1dcac84 to 72e40cd Compare September 23, 2024 09:07
@evalott100
Copy link
Contributor Author

evalott100 commented Sep 23, 2024 •
edited
Loading

@coretl I think I should maybe make each field Optional in LimitsRange (as opposed to NotRequired). EPICS does allow for low/high limits without high/low being set?

@evalott100 evalott100 force-pushed the 309-add-not-required-things-to-datakey-schema branch 2 times, most recently from 49a4ab1 to 3ce1f5f Compare September 23, 2024 09:34
@evalott100
added enum choices to DataKey
2d0bf2d 
Also made each field in `LimitsRange` optional.
@evalott100 evalott100 force-pushed the 309-add-not-required-things-to-datakey-schema branch from 3ce1f5f to 2d0bf2d Compare September 23, 2024 12:23
@evalott100 evalott100 merged commit 3c9ce76 into main Sep 23, 2024
12 of 13 checks passed
@evalott100 evalott100 deleted the 309-add-not-required-things-to-datakey-schema branch September 23, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiamondJoseph DiamondJoseph DiamondJoseph approved these changes

@coretl coretl Awaiting requested review from coretl

Assignees

@evalott100 evalott100

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Please export Dtype in event_model.__all__ Add NotRequired things to the DataKey schema
3 participants
@evalott100 @coretl @DiamondJoseph