yaml-language-server

yaml-language-server copied to clipboard

What does this PR do?

This treats descriptions in OpenAPI documents as markdown as per https://spec.openapis.org/oas/latest.html#rich-text-formatting

What issues does this PR fix or reference?

It fixes a specific, yet common use case for https://github.com/redhat-developer/vscode-yaml/issues/454

Is it tested? How?

A test was added.

Coverage increased (+0.1%) to 78.529% when pulling 466a7edbfbe83506f704209687205557d60f0b5c on remcohaszing:openapi-markdown-descriptions into 6df37d38dfe7bedce12aee4175fbe7da05b8e333 on redhat-developer:main.

Would you also accept if I add a property to schema settings to force enable markdown descriptions? I see now that this is very easy.

  export interface SchemasSettings {
    priority?: SchemaPriority;document then highest priority wins
    fileMatch: string[];
+   markdownDescriptions?: boolean
    schema?: unknown;
    uri: string;
  }

        if (schema && node && !schema.errors.length) {
-         const descriptionsAreMarkdown = 'openapi' in schema.schema || 'swagger' in schema.schema;
+         const descriptionsAreMarkdown = schema.markdownDescriptions || 'openapi' in schema.schema || 'swagger' in schema.schema;
          const matchingSchemas = doc.getMatchingSchemas(schema.schema, node.offset);

This would make it possible to specify JSON schema descriptions are markdown in monaco-yaml. Also although it wouldn’t resolve the referenced issue yet, but it would be a first step towards a fix.

Can we take a different approach and make toMarkdown function more clever so that it does not make the changes that it applies when it is not necessary?

toMarkdown could simply take descriptionsAreMarkdown as a second argument, and return the raw input if it’s this property is true. Although IMO this doesn’t make the code simpler or more readable.

I was actually thinking if we could make toMarkdown to detect if the string is already Markdown and skip modifying it. Unfortunately, I was not successful to spot an easy way to do that with my glorious 3 min spend on Google :)

I was actually thinking if we could make toMarkdown to detect if the string is already Markdown and skip modifying it. Unfortunately, I was not successful to spot an easy way to do that with my glorious 3 min spend on Google :)

I’m pretty sure it’s not. :disappointed: It would have been awesome though.

I have also been thinking maybe all descriptions could be treated as markdown, but that would mess up code sample for example.

For example, the following probably represents a code sample in plain text:

This is a code sample
function foo() {
  console.log('bar');
}

However, in markdown, this is equivalent to:

This is a code sample function foo() { console.log('bar'); }

I am a bit reluctant on this one because we are introducing a behavior based on schema. We did that in the early times with kubernetes and it has been haunting us since. We are actually planning to remove it. Can we think of a way to introduce this without yaml-language-server being aware of the schema?

The alternative is what I proposed in https://github.com/redhat-developer/yaml-language-server/pull/536#issuecomment-912499323. This makes the hover provider only aware of whether or not it should treat descriptions as markdown, but not why. This moves the responsibility of determining whether or not a schema has markdown descriptions up.

I am more comfortable with that one, instead of yaml-language-server being aware of the schemas. I am afraid the list of schemas may grow out of hand quickly.

I looked into moving this check up, but I’m not sure where exactly to put this then. I prefer keeping this as-is for now, as I know this works.