elastic / elasticsearch-specification

Elasticsearch full specification
Apache License 2.0
120 stars 69 forks source link

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

Open pquentin opened 1 month ago

pquentin commented 1 month ago

🚀 Feature Proposal

Descriptions in the specification should have the following format:

Motivation

The motivation is that OpenAPI that distinguishes between summary and description. Today, we try to split on the first sentence (see https://github.com/elastic/elasticsearch-specification/pull/2608 and https://github.com/elastic/elasticsearch-specification/pull/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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

l-trotta commented 1 month ago

@shainaraskas could you let me know if this works?

shainaraskas commented 1 month ago

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

l-trotta commented 1 month ago

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