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

JSDoc endpoint comments should be explicitly split in summary and description #2757

Open
pquentin opened this issue Aug 1, 2024 · 7 comments
Assignees

Comments

@pquentin
Copy link
Member

pquentin commented Aug 1, 2024

🚀 Feature Proposal

Descriptions in the specification should have the following format:

  • A one-line summary of the endpoint, without markup. Example: "Convert an index alias to a data stream."
  • An empty line (is that necessary?)
  • A description, using GitHub Flavored Markdown

Motivation

The motivation is that OpenAPI that distinguishes between summary and description. Today, we try to split on the first sentence (see #2608 and #2717), but having the source format correct will avoid using heuristics and will make sure we only use Markdown in the description.

The goal is not to migrate everything now, but document the ideal format for us.

Example

/**
 * Convert an index alias to a data stream.
 * 
 * Converts an index alias to a data stream.
 * You must have a matching index template that is data stream enabled.
 * The alias must meet the following criteria:\n
 * - The alias must have a write index.\n
 * - All indices for the alias must have a `@timestamp` field mapping of a `date` or `date_nanos` field type.\n
 * - The alias must not have any filters.\n
 * - The alias must not use custom routing.\n\n
 * If successful, the request removes the alias and creates a data stream with the same name.
 * The indices for the alias become hidden backing indices for the stream.
 * The write index for the alias becomes the write index for the stream.
 */

If we agree, I'll open a pull request to document this in https://github.com/elastic/elasticsearch-specification/blob/main/docs/doc-comments-guide.md.

@shainaraskas
Copy link
Contributor

shainaraskas commented Aug 1, 2024

This is maybe not completely relevant to the core issue, but I wanted to raise it just in case:

Markdown requires newlines for paragraph breaks, and the output currently escapes newline markup (\n becomes \\n and is visible in the output, at least in the openapi spec). Ideally, we'd still be able to use newlines in the backend markup like we do today:

/**
 * Convert an index alias to a data stream.
 * 
 * Converts an index alias to a data stream.
 * You must have a matching index template that is data stream enabled.
 *
 * The alias must meet the following criteria:
 *
 * - The alias must have a write index.
 * - All indices for the alias must have a `@timestamp` field mapping of a `date` or `date_nanos` field type.
 * - The alias must not have any filters.
 * - The alias must not use custom routing.
 *
 * If successful, the request removes the alias and creates a data stream with the same name.
 * The indices for the alias become hidden backing indices for the stream.
 * The write index for the alias becomes the write index for the stream.
 */

All that being said, I think an empty line between the summary and description makes things clearer 👍

@flobernd
Copy link
Member

flobernd commented Aug 1, 2024

I agree with @shainaraskas.

Markdown requires newlines for paragraph breaks

This is the main reason why I suggested this specific format. That allows us the parse the markdown and take the first paragraph node as the summary. Otherwise we would have to detect the end of the first sentence or to look for the first line break in the first paragraph etc.

the output currently escapes newline markup

This might be an issue in the OAPI converter. The line break must be escaped to be able to output it as a valid JSON string in the schema.json. The OAPI generator should unescape this before using the string in the OAPI Yaml. cc @l-trotta

@l-trotta l-trotta self-assigned this Aug 2, 2024
@l-trotta
Copy link
Contributor

l-trotta commented Aug 2, 2024

checking the OAPI converter.
so for the example description provided by @shainaraskas the OAPI output currently looks like:

"summary": "Convert an index alias to a data stream",
"description": "Converts an index alias to a data stream.\nYou must have a matching index template that is data stream enabled.\n\nThe alias must meet the following criteria:\n\n- The alias must have a write index.\n- All indices for the alias must have a `@timestamp` field mapping of a `date` or `date_nanos` field type.\n- The alias must not have any filters.\n- The alias must not use custom routing.\n\nIf successful, the request removes the alias and creates a data stream with the same name.\nThe indices for the alias become hidden backing indices for the stream.\nThe write index for the alias becomes the write index for the stream.",

what's the desired output?

@shainaraskas
Copy link
Contributor

@l-trotta the output is currently correct because I'm using spaces rather than typing \n

@l-trotta
Copy link
Contributor

l-trotta commented Aug 5, 2024

@shainaraskas could you let me know if this works?

@shainaraskas
Copy link
Contributor

@l-trotta yes, \n is being handled as expected in openapi output in that branch! 🎉

@l-trotta
Copy link
Contributor

l-trotta commented Aug 6, 2024

great! I'll do a few more tests to see if we can make the process more generic for every special symbol

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

No branches or pull requests

4 participants