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

Add OpenAPI schemas from optimade-python-tools v0.22.1 #8

Merged
merged 4 commits into from
Mar 6, 2023
Merged

Add OpenAPI schemas from optimade-python-tools v0.22.1 #8

merged 4 commits into from
Mar 6, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Aug 12, 2022 •
edited
Loading

Various improvements to the OpenAPI schemas:

  • cosmetic changes and addition of examples fields (good for swagger docs) (and annoying reordering of fields)
  • different return type where appropriate (application/json -> application/vnd.api+json)
  • additional custom fields for units, queryability and support levels
  • improved validation of lists types with min/max items and better regexps for chemical formulae
  • removal of default values and minimum values for page_number (see Add OpenAPI schemas from optimade-python-tools v0.22.1 #8 (comment))
  • fixes for page_above and page_below type

@ml-evs ml-evs mentioned this pull request Aug 15, 2022
Merged
This was referenced Oct 25, 2022
Merged
Closed
@ml-evs
Copy link
Member Author

ml-evs commented Oct 26, 2022

This discussion above should be resolved by 3cf0e77, with the schemas that will be released shortly under optimade-python-tools v0.19.5 (not yet merged).

@merkys This basically reverts the page_number related changes in this PR (and pushes validation into server-specific code for us) -- let me know what you think!

@ml-evs ml-evs requested review from blokhin and merkys October 26, 2022 09:41
merkys
merkys reviewed Oct 28, 2022
View reviewed changes
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

I am happy about reduced constraints for page_number, thanks!

However, some of the new constraints for formulas do not stem from the specification, but rather from the changes I have made in Materials-Consortia/optimade-python-tools#986 without having them properly discussed, as I did after the fact in Materials-Consortia/OPTIMADE#388. I will ping the thread to hopefully clean up the mess I made 😅

openapi/v1.1.0/optimade.json Show resolved Hide resolved
openapi/v1.1.0/optimade.json Outdated
},
"chemical_formula_reduced": {
"title": "Chemical Formula Reduced",
"pattern": "^([A-Z][a-z]?\\d*)*$",
"pattern": "^([A-Z][a-z]?([2-9]|[1-9]\\d+)?)+$",
Copy link
Member

Choose a reason for hiding this comment

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

The new pattern seems to reflect constraints I have suggested in Materials-Consortia/OPTIMADE#388. However, the issue does not seem resolved and constraints are not explicitly expressed in OPTIMADE specification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for looking at this so carefully @merkys, I'll add my comments to that issue

@merkys merkys mentioned this pull request Oct 28, 2022
Open
@ml-evs ml-evs changed the title Add OpenAPI schemas from optimade-python-tools v0.19.1 Add OpenAPI schemas from optimade-python-tools v0.20.1 Dec 6, 2022
@ml-evs ml-evs force-pushed the ml-evs/update_openapi_schemas branch from 7e999cb to 3cf0e77 Compare December 6, 2022 23:22
@ml-evs ml-evs mentioned this pull request Dec 6, 2022
Draft
@ml-evs ml-evs changed the title Add OpenAPI schemas from optimade-python-tools v0.20.1 Add OpenAPI schemas from optimade-python-tools v0.22.1 Mar 3, 2023
@ml-evs
Copy link
Member Author

ml-evs commented Mar 3, 2023

I think this can be safely merged now, after undoing the additional constraints on chemical formulae. This PR also now adds fixes associated with page_above and page_below that was previously incorrect in optimade-python-tools.

merkys
merkys approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

Looks good!

JPBergsma
JPBergsma approved these changes Mar 6, 2023
View reviewed changes
Copy link

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

LGTM

@ml-evs ml-evs merged commit 801912e into master Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merkys merkys merkys approved these changes

@JPBergsma JPBergsma JPBergsma approved these changes

@blokhin blokhin Awaiting requested review from blokhin

@rartino rartino Awaiting requested review from rartino

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ml-evs @blokhin @merkys @JPBergsma