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 to single-entry endpoints #254

Merged
merged 2 commits into from
Feb 19, 2020
Merged

Add include query parameter to single-entry endpoints #254

merged 2 commits into from
Feb 19, 2020

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Feb 10, 2020 •
edited
Loading

Closes #251

Add a sentence concerning the include query parameter to the section Single Entry URL Query Parameters stating it is OPTIONAL for both clients and API implementations (as is also the case for the entry endpoints).

@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 Feb 10, 2020
merkys
merkys requested changes Feb 10, 2020
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 suggest marking up the URL query parameter name as ":query-param:include", as seems to be done at least in some parts in our RST document. It would be nice to unify the markup through the document.

rartino
rartino requested changes Feb 11, 2020
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 support this change, with the addition of @merkys. However, please don't merge until #255 is resolved and a new version number has been merged into develop.

@CasperWA CasperWA 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 Feb 19, 2020
@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 Feb 19, 2020
@CasperWA CasperWA requested review from merkys and rartino February 19, 2020 10:33
@CasperWA
Copy link
Member Author

(...) please don't merge until #255 is resolved and a new version number has been merged into develop.

This issue has now been resolved.

merkys
merkys approved these changes Feb 19, 2020
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.

OK with me!

rartino
rartino approved these changes Feb 19, 2020
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.

Look good.

@rartino rartino merged commit 4870189 into Materials-Consortia:develop Feb 19, 2020
@CasperWA CasperWA deleted the fix_251_include_in_single_entry branch February 19, 2020 11:09
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

@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.

Should include be part of the single entry URL query parameters?
3 participants
@CasperWA @rartino @merkys