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 include query parameter and be more explicit about top-level included #219

Merged
merged 6 commits into from
Dec 4, 2019
Merged

Add include query parameter and be more explicit about top-level included #219

merged 6 commits into from
Dec 4, 2019

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Dec 3, 2019

Closes #212
Closes #183

@CasperWA
Add include query parameter.
7fe6e88 
Be more specific about the top-level `included` field.
@CasperWA CasperWA added the 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 label Dec 3, 2019
CasperWA
CasperWA commented Dec 3, 2019
View reviewed changes
optimade.rst Outdated Show resolved Hide resolved
merkys
merkys requested changes Dec 3, 2019
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.

Thanks @CasperWA for putting this down! To me the PR seems to express all the requirements that I remember from our previous Web meeting, and it reflects how I would (personally) implement the JSON API specification. I've left just a couple of remarks.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
CasperWA and others added 3 commits December 3, 2019 14:12
@CasperWA @merkys
Fix typing error
cc7754b 
Co-Authored-By: Andrius Merkys <andrius.merkys@gmail.com>
@CasperWA @merkys
Fix typing error
341f7df 
Co-Authored-By: Andrius Merkys <andrius.merkys@gmail.com>
@CasperWA
Update according to review by @merkys
bece8b5
@CasperWA CasperWA requested a review from merkys December 3, 2019 13:29
merkys
merkys previously approved these changes Dec 3, 2019
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.

Good to go!

ml-evs
ml-evs previously approved these changes Dec 3, 2019
View reviewed changes
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.

This seems nice and clear to me, good work @CasperWA & @merkys.

@CasperWA
Copy link
Member Author

CasperWA commented Dec 3, 2019

Before merging, I would like to hear a consideration from @rartino, @fawzi, @sauliusg or @giovannipizzi concerning the inclusion of ?include= in the spec as a valid way of explicitly leaving out all related resource objects from the top-level included field.

rartino
rartino requested changes Dec 4, 2019
View reviewed changes
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.

I've made a comment and a suggestion below

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@rartino rartino added PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR and removed 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 Dec 4, 2019
@CasperWA @rartino
Clarify empty include note
8565af7 
Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>
@CasperWA CasperWA dismissed stale reviews from ml-evs and merkys via 8565af7 December 4, 2019 11:00
@CasperWA CasperWA added 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 and removed PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR labels Dec 4, 2019
@rartino rartino merged commit 141670a into Materials-Consortia:develop Dec 4, 2019
@CasperWA CasperWA deleted the close_212_update_included branch December 5, 2019 11:47
@CasperWA CasperWA mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rartino rartino rartino approved these changes

@merkys merkys merkys approved these changes

@fawzi fawzi Awaiting requested review from fawzi

@sauliusg sauliusg Awaiting requested review from sauliusg

@giovannipizzi giovannipizzi Awaiting requested review from giovannipizzi

@ml-evs ml-evs Awaiting requested review from ml-evs

Assignees
No one assigned
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update requirements for top-level "included" Handle query parameter 'include' as mandated by JSON API
4 participants
@CasperWA @rartino @ml-evs @merkys