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 type key to properties (simple) #282

Merged
merged 3 commits into from
Jun 13, 2020

Conversation

CasperWA
Copy link
Member

This addresses #269, but does not close it. That will be up to #270.

The type key will hold the value of the outermost type of the property it describes, i.e., for species, which is described as a "list of dictionaries", the type will be "list".

The value of type MUST be one of the OPTIMADE data types described here in the specification.

@CasperWA CasperWA added status/approved Consensus has been reached to implement this as described in the top comment. priority/high There is a consensus that resolving this should be prioritized. PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing labels Jun 12, 2020
@CasperWA CasperWA added this to the 1.0 release milestone Jun 12, 2020
giovannipizzi
giovannipizzi previously approved these changes Jun 12, 2020
Copy link
Contributor

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Looks useful to me - only doubt I have now is about the units being just a string, especially when you e.g. have composite units: is speed "m/s" or "m s^-1" something else? I'll move this to a different issue

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

I'm happy with this as is, but do you think there should be a discussion of the type field in the section about provider-specific types? i.e. "Providers MUST/SHOULD provide the type of custom fields at the info/entry endpoint", or some such? Otherwise I don't see what value is gained from this PR alone for clients trying to display custom fields (as probably no-one will implement this field).

@CasperWA
Copy link
Member Author

I'm happy with this as is, but do you think there should be a discussion of the type field in the section about provider-specific types? i.e. "Providers MUST/SHOULD provide the type of custom fields at the info/entry endpoint", or some such? Otherwise I don't see what value is gained from this PR alone for clients trying to display custom fields (as probably no-one will implement this field).

That would be great, but considering so many of the properties are already not a MUST, I think it's hard to justify this point.

However, we could always add a RECOMMENDED?

@rartino
Copy link
Contributor

rartino commented Jun 12, 2020

When we later upgrade this field to be a more complete type description; will we have to consider this a backwards-breaking change (i.e., new major version number) or a minor version?

How about we write in this text to that if the value isn't a simple OPTIMADE Datatype, the client should ignore the field?

Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Alright, I'll make my suggestion a concrete edit.

optimade.rst Outdated Show resolved Hide resolved
Add new :type: rst code for OPTIMADE data types.
Require database-provider-specific properties to be listed in the
introspective entry listing info endpoints.
@CasperWA
Copy link
Member Author

In the latest commit I've added the following:

  • A database-provider-specific property MUST be listed in its respective entry listing /info endpoint.
  • Future-proofing the type field.
  • New :type: rst role to be used when writing OPTIMADE data types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing priority/high There is a consensus that resolving this should be prioritized. status/approved Consensus has been reached to implement this as described in the top comment.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants