apiaryio / api-blueprint

API Blueprint
https://apiblueprint.org
MIT License
8.64k stars 2.14k forks source link

Implemented Request Parameters (RFC0004) #293

Closed pksunkara closed 3 years ago

pksunkara commented 8 years ago

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 Reviewable

zdne commented 8 years ago

@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

zdne commented 8 years ago

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

kylef commented 8 years ago

Remember to do the following:

pksunkara commented 8 years ago

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

zdne commented 8 years ago

@pksunkara why are you mentioning it https://github.com/apiaryio/api-blueprint/pull/293#issuecomment-167579599 ? (besides it should be "Do not merge" + "Passed review" – one is given by the author other by the reviewer)

pksunkara commented 8 years ago

@zdne I was replying to this line from you.

Also this should have do not merge label until it is implemented

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 :smile:

pksunkara commented 8 years ago

I have removed the URI parameters from Resource Model since it is soon to be deprecated anyway. This PR is ready for review now.

pksunkara commented 8 years ago

Addressed.

kylef commented 8 years ago

This looks good to me :+1:

pksunkara commented 8 years ago

@w-vi Fixed.