-
Notifications
You must be signed in to change notification settings - Fork 28
fix(133,137): correct Create/Apply HTTP response codes to 201 #406
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
e0cc8e3
2e66397
52d31de
34582b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,7 +196,7 @@ paths: | |
| $ref: '#/components/schemas/isbn' | ||
| required: true | ||
| responses: | ||
| '200': | ||
| '201': | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these examples are generated via aepc: https://github.com/aep-dev/aepc. It would be great to update that first. But I can do so after to align if that's too much effort.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll give it a shot.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aep-dev/aep-lib-go#15 - I think this needs to happen first unless I'm mistaken?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep! thanks for drafting that, and it's a |
||
| content: | ||
| application/json: | ||
| schema: | ||
|
|
@@ -269,7 +269,7 @@ paths: | |
| $ref: '#/components/schemas/publisher' | ||
| required: true | ||
| responses: | ||
| '200': | ||
| '201': | ||
| content: | ||
| application/json: | ||
| schema: | ||
|
|
@@ -354,7 +354,13 @@ paths: | |
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/publisher' | ||
| description: Successful response | ||
| description: Successful response (updated) | ||
| '201': | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/publisher' | ||
| description: Successful response (created) | ||
| /publishers/{publisher_id}/books: | ||
| get: | ||
| description: List method for book | ||
|
|
@@ -411,7 +417,7 @@ paths: | |
| $ref: '#/components/schemas/book' | ||
| required: true | ||
| responses: | ||
| '200': | ||
| '201': | ||
| content: | ||
| application/json: | ||
| schema: | ||
|
|
@@ -516,7 +522,13 @@ paths: | |
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/book' | ||
| description: Successful response | ||
| description: Successful response (updated) | ||
| '201': | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/book' | ||
| description: Successful response (created) | ||
| /publishers/{publisher_id}/books/{book_id}/editions: | ||
| get: | ||
| description: List method for book-edition | ||
|
|
@@ -579,7 +591,7 @@ paths: | |
| $ref: '#/components/schemas/book-edition' | ||
| required: true | ||
| responses: | ||
| '200': | ||
| '201': | ||
| content: | ||
| application/json: | ||
| schema: | ||
|
|
||
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 don't think we should include this, as a convention it would litter the spec with a bunch of footnotes. remove?
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.
The great thing about editions is that the old text lives in the aep-2026 branch.
It might be worth keeping a CHANGELOG of things we've changed, but we can also build that from git logs
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.
Removed the note. It does clutter it so I agree with removing it, I just wanted to prompt all of us to think through how this could be a breaking change in some cases. While we can accept 200 for backward compatibility, new things being created would not necessarily know about that.
I'm not super worried about it at the moment, but this change could have implications for the lib, the linter, aepc, aepbase, etc., and anyone who uses them.
Our user base is not super large right now and so we can get away with making changes like these, which is nice. I think we just press forward, test things out, and fix them where we can.
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.
Sounds good! the worst case scenario is we re-add it.