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

[Proposal] Update API template #7709

Merged
merged 11 commits into from
Jul 24, 2024
Merged

[Proposal] Update API template #7709

merged 11 commits into from
Jul 24, 2024

Conversation

Naarcha-AWS
Copy link
Collaborator

@Naarcha-AWS Naarcha-AWS commented Jul 15, 2024

As I've closing the additional API content gaps, there are a few scenarios I've encountered that the current API template does not account for, such as multiple examples or when additional complex descriptions are required such as the Rollover API. Therefore, I would like to propose the following changes to the API template:

  • Move the "Required permissions" section after Path and HTTP methods.
  • Simplify the default table settings by removing "data" from "data type". This is also more accurate since the type technically isn't "data" every time. Instead, the "type" can indicate a style or tweak to an operation, such as a Boolean.
  • Change "Request fields" to "Request body options". Request body options is more in line with the OpenAPI standard and further distinguishes these options from path and query parameters. For more information, see Describing Request Body.
  • Change "Response fields" to "Response body options".
  • Change "Example requests" and "Example responses" from H4 to H2 headers. This makes the examples more accessible and even with the rest of the template sections, and allows for the addition of subheadings in cases where an API contains multiple examples.
  • Add a note that accounts for examples which might require additional explanation.

Version

All.

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Updates the API template to allow for more dynamic formatting.

Signed-off-by: Naarcha-AWS <[email protected]>
Copy link

Thank you for submitting your PR. The PR states are In progress (or Draft) -> Tech review -> Doc review -> Editorial review -> Merged.

Before you submit your PR for doc review, make sure the content is technically accurate. If you need help finding a tech reviewer, tag a maintainer.

When you're ready for doc review, tag the assignee of this PR. The doc reviewer may push edits to the PR directly or leave comments and editorial suggestions for you to address (let us know in a comment if you have a preference). The doc reviewer will arrange for an editorial review.

@kolchfa-aws
Copy link
Collaborator

kolchfa-aws commented Jul 18, 2024

Overall, I don't see much value in doing this. This will require changing many files and is clear as is.

* Move the "Required permissions" section after Path and HTTP methods.

Don't particularly have an opinion on this one but I think it breaks the flow when this is in the middle.

* Simplify the default table settings by removing "data" from "data type". This is also more accurate since the type technically isn't "data" every time. Instead, the "type" can indicate a style or tweak to an operation, such as a Boolean.

Data type is a standard CS concept. Boolean is a data type https://en.wikipedia.org/wiki/Boolean_data_type.

* Change "Request fields" to "Request body options". Request body options is more in line with the OpenAPI standard and further distinguishes these options from path and query parameters. For more information, see [Describing Request Body](https://swagger.io/docs/specification/describing-request-body/).

"Options" is not standard language. There is no mention of the word "options" at the provided link. Maybe, to clarify further, we can name this "Request body fields".

* Change "Response fields" to "Response body options".

Options imply the ability to change. There are no options that users can change in the response. The response contains the fields that come from the server.

* Change "Example requests" and "Example responses" from H4 to H2 headers. This makes the examples more accessible and even with the rest of the template sections, and allows for the addition of subheadings in cases where an API contains multiple examples.

* Add a note that accounts for examples which might require additional explanation.

We already have some pages that have multiple examples. For example, https://opensearch.org/docs/latest/api-reference/index-apis/force-merge/. We normally clarify the example type in the heading like "Example request: Force merge multiple indexes". These are self-explanatory, I think. Changing from H4 to H2 will make them visible in the right navigation, which is not ideal, given that if you clarify the example type within the header, the header becomes long.

@Naarcha-AWS
Copy link
Collaborator Author

@kolchfa-aws:

As a general note, most of the APIs on our site do not use the template. Here are some examples:

Discussing this while the majority of the API content gaps I'm closing are still in review makes sense to me. Whether we change the template or not, we'll still have to go through and standardize to the template across the board.

Responding to your other comments:


"Options" is not standard language. There is no mention of the word "options" at the provided link. Maybe, to clarify further, we can name this "Request body fields".

That's fine by me.


Options imply the ability to change. There are no options that users can change in the response. The response contains the fields that come from the server.

Noted, does "Response body fields?" work for you?


Changing from H4 to H2 will make them visible in the right navigation, which is not ideal, given that if you clarify the example type within the header, the header becomes long.

This is the change I feel most passionately about. Its a bad user experience to bury the examples/example responses under a header that's inaccessible from the right nav. For an example of this, see the Force Merge page. Not only does a header for the example requests not exist, but I have to scroll all the way down to see a copyable example.

Having the examples directly accessible in the right navigation allows me to use the reference how I see fit. Instead of scrolling, I can click on the example that most pertains to me from the right nav and copy it immediately, like on the https://opensearch.org/docs/latest/api-reference/render-template/ page.

Furthermore, the example request and example response should have a clearer delineation between the two. Using Force Merge again, the response blends in with the examples, so it's not immediately clear that I'm looking at a Response Body visually.

As far as the header length is concerned, we shouldn't be making headers that are sentence-length anyways, even as H4-level headers. Giving both the Example request and Example response an H2 Header removes the need to prepend the H4 header with "Example response:" greatly reducing the length of the header to begin with.

There are multiple ways I'm okay changing this though:

  • The currently proposed method. See Render template
  • Have ##Examples be an H2 Header with "Requests" and "Responses" as H3s. If a request has multiple examples, you could use the H4 Headers, which would make them inaccesbile from the right nav. See the Multi-search API.

Thoughts? Both of those solutions solve the usability issue I described.

@kolchfa-aws
Copy link
Collaborator

@Naarcha-AWS "Response body fields" works for me and matches "Request body fields". I'm also fine with the example you gave in the Render Template API. Less of a fan of the Multi-search API. I like "Example request" (singular) and "Example response" and the various types within the "Example request". This separates them a little more. Note that implementing this for all API pages is not a simple find/replace and is best done with a regex and a script.

@kolchfa-aws
Copy link
Collaborator

BTW, this probably should have been an issue and not a PR 😄

@Naarcha-AWS
Copy link
Collaborator Author

@kolchfa-aws: I implemented the changes we agreed on. Let me know if there's anything else you would like to adjust or discuss.

@kolchfa-aws
Copy link
Collaborator

@Naarcha-AWS Please revert Type back to Data type. As explained, "data type" is a common CS concept and should be kept as is to avoid confusion. https://en.wikipedia.org/wiki/Data_type
Also, I think if we change the template, we should change all API files in the same PR so they follow the template.

@kolchfa-aws
Copy link
Collaborator

Also, I see you switched the "Example response" and "Response fields" sections. I think this is worse than the current order because when users see an example request, they expect that the example response is right after. Then they can explore the response fields in the "Response fields" section.

@Naarcha-AWS
Copy link
Collaborator Author

Also, I see you switched the "Example response" and "Response fields" sections. I think this is worse than the current order because when users see an example request, they expect that the example response is right after. Then they can explore the response fields in the "Response fields" section.

I don't personally think it makes much of a difference, although I find it a little odd to see the response fields after as opposed to before. That being said, ok switching this back.

@Naarcha-AWS
Copy link
Collaborator Author

Naarcha-AWS commented Jul 22, 2024

@kolchfa-aws: Fixed the order and added back Data Type, per your request. Once you sign off, I can go ahead and make sure the pages follow the template

@kolchfa-aws
Copy link
Collaborator

If there are multiple example requests, there should be an intro sentence before the first h3 to avoid stacked headings. Do you want to provide that intro sentence so it's standardized? I think we should change all API files to follow the new template in this PR to ensure consistency.

@hdhalter
Copy link
Contributor

If there are multiple example requests, there should be an intro sentence before the first h3 to avoid stacked headings. Do you want to provide that intro sentence so it's standardized? I think we should change all API files to follow the new template in this PR to ensure consistency.

Is it worth doing now, or should we wait until after we have full coverage? I'm in favor of waiting until we have full coverage, unless the changes are minimal.

@kolchfa-aws
Copy link
Collaborator

I would just change the heading level 4 in Example request/Example response headings to heading level 2. The other changes provide minimal value and require more work. Just changing a heading level is trivial.

@Naarcha-AWS
Copy link
Collaborator Author

If there are multiple example requests, there should be an intro sentence before the first h3 to avoid stacked headings. Do you want to provide that intro sentence so it's standardized? I think we should change all API files to follow the new template in this PR to ensure consistency.

Something along the lines, "The following example request shows _____"?

@hdhalter hdhalter added 2 - In progress Issue/PR: The issue or PR is in progress. and removed discuss labels Jul 23, 2024
Copy link
Contributor

@hdhalter hdhalter left a comment

Choose a reason for hiding this comment

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

LGTM

@Naarcha-AWS Naarcha-AWS merged commit 279237d into main Jul 24, 2024
8 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 24, 2024
* [Proposal] Update API template

Updates the API template to allow for more dynamic formatting.

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Update API_TEMPLATE.md

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Update API_TEMPLATE.md

Signed-off-by: Naarcha-AWS <[email protected]>

* Add H3 level examples.

Signed-off-by: Naarcha-AWS <[email protected]>

* Make APIs conform to template

Signed-off-by: Archer <[email protected]>

* Fix cluster API templates

Signed-off-by: Archer <[email protected]>

* Fix link

Signed-off-by: Archer <[email protected]>

---------

Signed-off-by: Naarcha-AWS <[email protected]>
Signed-off-by: Archer <[email protected]>
(cherry picked from commit 279237d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
github-actions bot pushed a commit that referenced this pull request Jul 24, 2024
sandervandegeijn pushed a commit to sandervandegeijn/documentation-website that referenced this pull request Jul 30, 2024
* [Proposal] Update API template

Updates the API template to allow for more dynamic formatting.

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Update API_TEMPLATE.md

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Apply suggestions from code review

Signed-off-by: Naarcha-AWS <[email protected]>

* Update API_TEMPLATE.md

Signed-off-by: Naarcha-AWS <[email protected]>

* Add H3 level examples.

Signed-off-by: Naarcha-AWS <[email protected]>

* Make APIs conform to template

Signed-off-by: Archer <[email protected]>

* Fix cluster API templates

Signed-off-by: Archer <[email protected]>

* Fix link

Signed-off-by: Archer <[email protected]>

---------

Signed-off-by: Naarcha-AWS <[email protected]>
Signed-off-by: Archer <[email protected]>
Signed-off-by: Sander van de Geijn <[email protected]>
@Naarcha-AWS Naarcha-AWS deleted the api-template branch December 19, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In progress Issue/PR: The issue or PR is in progress. backport 2.15
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants