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

Add optional labels to float fields #174

Merged
merged 13 commits into from
May 28, 2020

Conversation

skearnes
Copy link
Collaborator

@skearnes skearnes commented May 27, 2020

Fixes #156

See the announcement and the docs.

Notes:

  • (Gotcha) Using CopyFrom to overwrite an existing message containing an optional field causes a segmentation fault. See the changes to message_helpers.py. This could be specific to messages in oneof groups.
  • Updated validations.py to not allow any UNSPECIFIED units or type values.
  • Changed PressureControl.type to PRESSURIZED in example_Perera.ipynb.

Questions:

  • Should we allow UNSPECIFIED as an enum value? Note that it is possible to label enum value fields as optional as well (and we could drop UNSPECIFIED entirely). If we did decide to make this change, we should make sure that the web form includes an extra "empty" value as the default that means "do not set anything in the generated proto" so we can tell the difference. Personally I think leaving UNSPECIFIED as a default value that is never actually used makes some sense since it avoids having to do the checks for explicit presence.

@skearnes skearnes marked this pull request as ready for review May 28, 2020 02:27
@skearnes skearnes requested a review from connorcoley May 28, 2020 02:27
@skearnes skearnes merged commit fa599e5 into open-reaction-database:master May 28, 2020
@skearnes skearnes deleted the ambig branch May 28, 2020 20:44
skearnes added a commit that referenced this pull request May 30, 2020
skearnes added a commit that referenced this pull request May 30, 2020
@skearnes skearnes mentioned this pull request May 31, 2020
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.

Ambiguous defaults in the schema
2 participants