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.

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

Add suggested changes from API guild discussion (POST usage) #791

Merged
merged 8 commits into from
Feb 29, 2024

Conversation

SmilyOrg
Copy link
Member

I took some liberties on the formatting/wording, but mostly it's as discussed in the internal doc.

chapters/http-requests.adoc Outdated Show resolved Hide resolved
chapters/http-requests.adoc Outdated Show resolved Hide resolved
chapters/http-requests.adoc Outdated Show resolved Hide resolved
@tkrop
Copy link
Member

tkrop commented Nov 29, 2023

I'm a bit hesitant to approve this, because the pull request majorly splits up the dense description from before into 4 subsections without providing additional clarification that wasn't clear from the before context. In my opinion the deeper nesting is not preferable here.

The wording improvements are fine and could be added to the before structure too.

@SmilyOrg
Copy link
Member Author

I'm a bit hesitant to approve this, because the pull request majorly splits up the dense description from before into 4 subsections without providing additional clarification that wasn't clear from the before context. In my opinion the deeper nesting is not preferable here.

The wording improvements are fine and could be added to the before structure too.

You're right, looking at it again it seems a bit too wordy. However, the previous dense description seemed a bit too overloaded, so there was a lot to interpret. Maybe there's a good structure somewhere in the middle? I'll have to take another look :)

Either way, I applied the suggestions, thanks!

Base automatically changed from improve-response-codes to main December 12, 2023 13:39
@tfrauenstein
Copy link
Member

👍

Copy link
Member

@tkrop tkrop left a comment

Choose a reason for hiding this comment

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

@SmilyOrg have left some comments for polishing.

chapters/http-requests.adoc Outdated Show resolved Hide resolved
chapters/http-requests.adoc Outdated Show resolved Hide resolved
chapters/http-status-codes-and-errors.adoc Outdated Show resolved Hide resolved
chapters/http-status-codes-and-errors.adoc Outdated Show resolved Hide resolved
chapters/http-status-codes-and-errors.adoc Outdated Show resolved Hide resolved
chapters/http-status-codes-and-errors.adoc Outdated Show resolved Hide resolved
chapters/http-status-codes-and-errors.adoc Outdated Show resolved Hide resolved
@SmilyOrg
Copy link
Member Author

SmilyOrg commented Feb 8, 2024

@tkrop this is ready for another look. Most of your comments were actually on a diff that was from a non-ideal merge, so I opened #795 for those. The diff should be cleaner now.

@tkrop
Copy link
Member

tkrop commented Feb 21, 2024

👍

@ePaul ePaul changed the title Add suggested changes from API guild discussion Add suggested changes from API guild discussion (POST usage) Feb 22, 2024
Comment on lines 142 to 151
* For multiple resources you {SHOULD} return {201} in the response, as long as
they are created atomically, meaning that either all the resources are created
or none of them are.
** You <<152>> if the request can partially fail, that is some of the resources
can fail to be created.
* If the response does not contain the created resource or a resource monitor,
the status code {SHOULD} be {201} and the URL to {GET} the resource {MUST} be
provided in the {Location} response header.
* If the {POST} is <<idempotent>>, the status code {SHOULD} be {201} if the
resource was created and {200} or {204} if the resource was updated.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether these last two bullet points are more applicable for the single-resource case? They certainly don't fit for the "return 207" case. Maybe move them as sub-bullet-points to the single-resource case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I'll do that

@SmilyOrg
Copy link
Member Author

@ePaul I added your suggestion, please check again!

@SmilyOrg
Copy link
Member Author

👍

1 similar comment
@ePaul
Copy link
Member

ePaul commented Feb 29, 2024

👍

@SmilyOrg SmilyOrg merged commit fe3ff88 into main Feb 29, 2024
2 checks passed
@SmilyOrg SmilyOrg deleted the post-suggestions branch February 29, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants