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

openAPI: Auto render parameter and response descriptions. #18936

Merged
merged 4 commits into from
Jun 22, 2021

Conversation

MSurfer20
Copy link
Member

@MSurfer20 MSurfer20 commented Jun 21, 2021

This is the preparatory PR for #18937, and adds Markdown extensions to render necessary openapi parameters in the follow-up PR.

Testing plan:

GIFs or screenshots:

As a goal of a common template, there
is a need for a tool to auto-generate
general description for all responses
directly from OpenAPI data.

This description is to be stored in
x-response-description field, and this
commit adds a markdown extesion to process the same.
As a goal of a common template, there
is a need for a tool to auto-generate
general description for all parameters
directly from OpenAPI data.

This description is to be stored in
x-response-description field, and this
commit adds a markdown extesion to process the same.
This commit modifies the templates to
auto-generate general descriptions of
parameters directly from the newly
added field of x-parameter-description
as a part of the goal of a common template.
This commit modifies the templates to
auto-generate general descriptions of
responses directly from the newly
added field of x-response-description
as a part of the goal of a common template.
@MSurfer20
Copy link
Member Author

@timabbott Checked the diff b/w older and newer raw HTML -- no difference

@@ -27,6 +27,8 @@

{generate_return_values_table|zulip.yaml|/realm/playgrounds:post}

{generate_response_description(/realm/playgrounds:post)}
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It appears the organization you've gone with is this:

## Parameters

{generate_api_arguments_table|zulip.yaml|/realm/playgrounds:post}

{generate_parameter_description(/realm/playgrounds:post)}

## Response

{generate_return_values_table|zulip.yaml|/realm/playgrounds:post}

{generate_response_description(/realm/playgrounds:post)}

What's the thinking behind the response descriptions sitting after the table of parameters?

It appears the actual docs have a mix of both styles. I'll look at the examples and think about this.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think after playing with this, the approach of a note at the end is probably fine. Posted detailed comments on individual items in the other PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears the organization you've gone with is this:

## Parameters

{generate_api_arguments_table|zulip.yaml|/realm/playgrounds:post}

{generate_parameter_description(/realm/playgrounds:post)}

## Response

{generate_return_values_table|zulip.yaml|/realm/playgrounds:post}

{generate_response_description(/realm/playgrounds:post)}

What's the thinking behind the response descriptions sitting after the table of parameters?

It appears the actual docs have a mix of both styles. I'll look at the examples and think about this.

@timabbott In addition to the already existing examples, I feel that we would more likely want to talk about common elements about parameters/responses after mentioning them all for the context, and hence the decision. Further, the already existing cases where descriptions occur before seem to be easily adjustable to the other format.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Yeah, I think this is fine for now. It might be better writing style to have these at the top in many cases, but we'll need to rewrite the text to do so, and the goal here is to migrate to a world where it'd be easy to just edit the default/base template anyway.

break
else:
done = True
return lines
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think it would be well worth deduplicating these extensions as a follow-up PR. The run code is the same for all of these aside from the regex and the render function, so I think we could have a base class that we subclass with different render functions and REGEX properties and delete a lot of duplicate code.

@timabbott timabbott merged commit 5f07c06 into zulip:master Jun 22, 2021
@timabbott
Copy link
Sponsor Member

Merged, thanks @MSurfer20!

md.preprocessors.register(
ResponseDescriptionPreprocessor(md, self.getConfigs()),
"generate_response_description",
531,
Copy link
Member

Choose a reason for hiding this comment

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

@MSurfer20, any particular reason to choose 531 as the priority number? As I can see that priority number 531 is already used by APITitlePreprocessor. And AFAIK, we keep these priority numbers distinct.
@timabbott can confirm this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants