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

Update the OptimadeMetaResponse to development schema #13

Merged
merged 7 commits into from Jun 12, 2019
Merged

Update the OptimadeMetaResponse to development schema #13

merged 7 commits into from Jun 12, 2019

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jun 12, 2019

This PR updates the OptimadeMetaResponse class to reflect the current state of the development schema, adding the OptimadeProvider class.

@ml-evs ml-evs requested review from dwinston and ltalirz June 12, 2019 10:06
ltalirz
ltalirz requested changes Jun 12, 2019
View reviewed changes
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks @ml-evs !
Just a few minor requests - some of which may not even be addressed in this PR.

optimade/server/models.py Outdated Show resolved Hide resolved
optimade/server/models.py Outdated Show resolved Hide resolved
optimade/server/models.py Outdated Show resolved Hide resolved
optimade/server/models.py Outdated Show resolved Hide resolved
optimade/server/models.py Outdated Show resolved Hide resolved
dwinston
dwinston suggested changes Jun 12, 2019
View reviewed changes
Copy link
Contributor

@dwinston dwinston left a comment

Choose a reason for hiding this comment

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

Hi @ml-evs, please refactor into the different models submodules.

@ml-evs ml-evs requested review from dwinston and ltalirz June 12, 2019 12:47
dwinston
dwinston suggested changes Jun 12, 2019
View reviewed changes
Copy link
Contributor

@dwinston dwinston left a comment

Choose a reason for hiding this comment

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

Looks great! Minor request: let's explicitly link to the 1.0 JSON API spec, e.g. https://jsonapi.org/format/1.0/#document-meta

@ml-evs
Copy link
Member Author

ml-evs commented Jun 12, 2019

Looks great! Minor request: let's explicitly link to the 1.0 JSON API spec, e.g. https://jsonapi.org/format/1.0/#document-meta

I guess we should do this in the spec too!

@ml-evs ml-evs requested a review from dwinston June 12, 2019 14:48
@ltalirz
Copy link
Member

ltalirz commented Jun 12, 2019

@ml-evs I left a few comments above, just wondering whether you were planning to address them / whether you think those don't belong here...

@ml-evs
Copy link
Member Author

ml-evs commented Jun 12, 2019

@ml-evs I left a few comments above, just wondering whether you were planning to address them / whether you think those don't belong here...

I've responded to each and resolved, while relevant I don't think they belong in this PR.

ltalirz
ltalirz approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @ml-evs !

@ml-evs ml-evs merged commit 50bc1b0 into master Jun 12, 2019
@CasperWA CasperWA deleted the updates-from-develop branch November 8, 2019 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ltalirz ltalirz ltalirz approved these changes

@dwinston dwinston dwinston approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@ml-evs @ltalirz @dwinston