Update OpenAPI Descriptions #230
Conversation
| deprecated: true | ||
| required: false | ||
| schema: | ||
| type: string |
There was a problem hiding this comment.
I'd suggest not to publicly document legacy parameters. They will find their way into the SDKs through code generation and instead of fewer people using them, it will be more.
There was a problem hiding this comment.
That's a good call. We want to document legacy parameters when they have been documented before, but not the ones that were never described since the existence of this document. I'll open a PR to filter those with a special attribute.
| READMEs support [custom media types](https://docs.github.com/rest/reference/repos#custom-media-types) for retrieving the raw content or rendered HTML. | ||
| tags: | ||
| - repos | ||
| operationId: repos/get-readme-from-alt-path |
There was a problem hiding this comment.
I think the operationId could be more descriptive. Changing it later will cause friction, as it will result in method name changes in the SDKs
| operationId: repos/get-readme-from-alt-path | |
| operationId: repos/get-readme-in-directory |
Also I wonder if the new endpoint is necessary? I wonder if you considered adding a dir query parameter to the existing repos/get-readme operation? It would be an optional parameter and hence not a breaking change?
There was a problem hiding this comment.
Agreed, this should be better described. Will fix.
There was a problem hiding this comment.
This is currently a publicly available endpoint (with traffic) that was never documented but also never deprecated. Adding it to the description in this case seems the like best course of action in this case.
There was a problem hiding this comment.
Agreed, this should be better described. Will fix.
you did not change the operationId. Can you still do that, if you prefer repos/get-readme-in-directory? The operation ID is not impacting users of the REST API, but better names will make the SDK method easier to understand
There was a problem hiding this comment.
Can you please change the operation ID to repos/get-readme-in-directory? The operation ID has path, but the query parameter is dir, it's not coherent.
The operation summary is still "Get a repository README" and externalDocs.url is https://docs.github.com/rest/reference/repos#get-a-repository-readme, both need to be changed, looks like a copy and paste oversight
| @@ -7984,6 +8005,17 @@ paths: | |||
| - "$ref": "#/components/parameters/org" | |||
| - "$ref": "#/components/parameters/per_page" | |||
| - "$ref": "#/components/parameters/page" | |||
| - name: exclude | |||
There was a problem hiding this comment.
Did you consider adding an exclude parameter globally to all endpoints?
In this PR you introduce exclude and excludedAttributes (see below). Maybe use one name to keep it coherent?
There was a problem hiding this comment.
@gr2m unfortunately these are not new parameters, but rather ones we've identified as missing. I agree with you on consistency, some we keep an eye for going forward.
| @@ -29119,6 +29310,18 @@ paths: | |||
| - "$ref": "#/components/parameters/enterprise" | |||
| - "$ref": "#/components/parameters/start_index" | |||
| - "$ref": "#/components/parameters/count" | |||
| - name: filter | |||
| description: filter results | |||
There was a problem hiding this comment.
I'll an open an issue to get a better description on this 👍
| @@ -69369,6 +69606,7 @@ components: | |||
| schema: | |||
| type: string | |||
| enum: | |||
| - '' | |||
There was a problem hiding this comment.
Just making sure: is this intentional?
There was a problem hiding this comment.
Yup, empty string is actually a valid input for this enum 😅
|
Thanks for catching those @gr2m, we did a lot of changes due to our whole test suite running against this description now. They all make this spec more accurate but we'll make the necessary changes to make descriptions/naming better per your suggestions 👍 |
shiftkey
deleted the
openapi-update-147cf7c7f19098caa9160c3dae17d19bf7e8c5d559cceabfb4ff9bdbee945a3c
branch
👋 humans. This PR updates the OpenAPI description with the latest changes.