-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Implemented Request Parameters (RFC0004) #293
Conversation
@pksunkara I would at least expect the link to the RFC in the description. Making it easier for person who reviews it might actually helps to get the review faster. Also this should have do not merge label until it is implemented |
@pksunkara please keep in mind providing context and clarity is what we all should aim for, looking at the first 20 lines of diffs I do not recall what RFC0004 was about. |
Remember to do the following:
|
- [`0-1` Schema section](#def-schema-section) | ||
- [`0-1` URI parameters section](#def-uriparameters-section) | ||
|
||
The URI parameters section included in this section describes the URI parameters attached to the request. They can be used to represent different responses based on different parameter values sent along with a request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a note mentioning that when you have two URI Parameter sections (request/action and a request level URI Parameter section) one overrides the other and there is no inheritance.
I think this might be ambiguous on how this is handled otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one overrides the other and there is no inheritance.
Why? Requests and responses can't have their own uriTemplate. So, they have to rely on the uriTemplate from the action (or above). So, why can't they inherit the parameters too?
I think they should definitely inherit the action parameters. We are adding the parameters in request to differentiate between different values of parameters which are inherited from the action.
An example would be:
### Search for a question [GET /questions{?query,limit}]
+ Parameters
+ query - Search query
+ limit: 20 (number, optional) - Maximum number of questions returned
+ Response 200 (application/json)
[
{ "question": "Hello?" },
{ "question": "Favourite programming language?" },
{ "question": "API Description language preference" }
]
+ Request A search limited to one result
+ Parameters
+ query: language
+ limit: 1
+ Response 200 (application/json)
[
{ "question": "Favourite programming language?" }
]
Here, query
in action parameters is left empty and it's value will be filled by the request parameters.
"Request Parameters" object inherit "Action Parameters" object. No new members will be added since the uriTemplate will be the same. It's just going to be the change of values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added an extra line saying the same.
@zdne We use "PR: Passed review" label to denote this PR as reviewed and should be merged only after implementation. Not the "PR: Do not merge" label. |
6e6a912
to
cbbf7f3
Compare
@pksunkara why are you mentioning it #293 (comment) ? (besides it should be "Do not merge" + "Passed review" – one is given by the author other by the reviewer) |
@zdne I was replying to this line from you.
I wanted to point out that we do not use "PR: do not merge" labels in this case because we generally use it to denote that a PR is not ready according to the author. Guess, we need more labels 😄 |
@@ -583,7 +584,7 @@ Resource representing **ACME Blog** posts. | |||
<a name="def-model-section"></a> | |||
## Resource model section | |||
- **Parent sections:** [Resource section](#def-resource-section) | |||
- **Nested sections:** [Refer to payload section](#def-payload-section) | |||
- **Nested sections:** [Refer to payload section](#def-payload-section), [`0-1` **URI Parameters** section](#def-uriparameters-section) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to add URI Parameters section to a resource model? Since a resource model doesn't have an associated URI (until it's referenced).
cbbf7f3
to
f15e9ff
Compare
I have removed the URI parameters from Resource Model since it is soon to be deprecated anyway. This PR is ready for review now. |
@@ -380,7 +381,7 @@ Furthermore, in API Blueprint context, a payload include its description, descri | |||
|
|||
A payload **may** have its media type associated. A payload's media type represents the metadata received or sent in the form of a HTTP `Content-Type` header. When specified a payload **should** include nested [Body section](#def-body-section). | |||
|
|||
This section **should** include at least one of the following nested sections: | |||
This section **should** include at least one of the following nested sections unless specified otherwise by the sections inheriting this section: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a comma before unless:
This section should include at least one of the following nested sections, unless specified otherwise by the sections inheriting this section:
f15e9ff
to
ed2a93f
Compare
Addressed. |
This looks good to me 👍 |
@@ -758,7 +759,7 @@ Retrieves the list of **ACME Blog** posts. | |||
<a name="def-request-section"></a> | |||
## Request section | |||
- **Parent sections:** [Action section](#def-action-section) | |||
- **Nested sections:** [Refer to payload section](#def-payload-section) | |||
- **Nested sections:** [Refer to payload section](#def-payload-section), [`0-1` **URI Parameters** section](#def-uriparameters-section) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the URI Parameters are bold?
ed2a93f
to
d726928
Compare
@w-vi Fixed. |
Things got a little bit more complex because of
Resource Model
:(Link to RFC: https://github.com/apiaryio/api-blueprint-rfcs/blob/master/accepted/0004-request-parameters.md
This change is