thim81 / openapi-format

Format an OpenAPI document by ordering, formatting and filtering fields.
MIT License
77 stars 14 forks source link

yaml document start characters `---` & comments not preserved #60

Open skarzi opened 1 year ago

skarzi commented 1 year ago

Hi :wave:

First of all thanks for developing this awesome lib!

I am using the openapi-format more like a validator (mainly because it removes comments like mentioned in https://github.com/thim81/openapi-format/issues/44), not as a formatted, and in some projects, we are using --- (a YAML document start sign - as described in 2.2 Structure), but running the openapi-format on such documents produces output without --- and it would be nice to preserve --- when it's present or add some option to always add/remove it from the file.

PS. https://github.com/thim81/asyncapi-format also does not preserve ---

thim81 commented 1 year ago

@skarzi Thanks for taking the time to report the issue.

Could you share a small sample of how you use the --- in the OpenAPI document?

That way I better understand the issue and I can create a fix/test for the unwanted behaviour.

For your info, openapi-format is using @stoplight/yaml as YAML parser, so it might be possible that the parser removes the ---.

(asyncapi-format is a bit behind openapi-format, but I do try to keep the feature set in sync as much as possible)

skarzi commented 1 year ago

I am just adding --- at the beginning of the file, e.g.

---
openapi: 3.0.0
info:
  version: 1.0.0
  description: Empty API description
  title: Empty API description
  license:
    name: MIT
  contact:
    email: contact@test.com
servers:
  - url: http://test.com/fantastic-api
paths: {}

and openapi-format simply strips --- from the API description.

But you are right that this issue is related to @stoplight/yaml (and precisely @stoplight/yaml-ast-parser where safeDump is defined).

thim81 commented 1 year ago

@skarzi Would you have some time to play around with some @stoplight/yaml safeDump options? To see if there is a setting that would keep the ---?

I look at the YAML spec but it is not clear what the --- does?

skarzi commented 1 year ago

I have checked and it's impossible to prepend --- to the document with @stoplight/yaml :(

However, I have found another YAML lib - https://github.com/eemeli/yaml - that supports document start characters as well as comments. Are there any special reasons you have chosen @stoplight/yaml lib to work with the YAML content? Because maybe we could try to migrate to eemli/yaml, so comments and --- will be supported. Another positive aspect about this lib is that it uses (and passes all) https://github.com/yaml/yaml-test-suite.

thim81 commented 1 year ago

@skarzi The main reason why we implemented @stopligth/yaml

https://github.com/thim81/openapi-format#stoplight-studio

We have adopted the YAML parsing style from Stoplight Studio, by leveraging the @stoplight/yaml package for handling the parsing of OpenAPI YAML files.

By using the Stoplight YAML parsing, the results will be slightly different from when using a normal YAML parsing library, like js-to-yaml. We appreciate the Stoplight Studio tool, since it is an excellent GUI for working with OpenAPI documents for non-OpenAPI experts who will be contributing changes. By adopting the Stoplight Studio YAML parsing, the potential risk of merge conflicts will be lowered, which is the main reason why we opted for using the @stoplight/yaml package.

skarzi commented 1 year ago

By using the Stoplight YAML parsing, the results will be slightly different from when using a normal YAML parsing library

What exactly are the differences?

By adopting the Stoplight Studio YAML parsing, the potential risk of merge conflicts will be lowered

Could you clarify this in more detail?

thim81 commented 1 year ago

The biggest difference was, the way multiline text was handled (| >) and quoting properties to make sure they are valid

The merge conflicts would be. If you open an OpenAPI file in Stoplight studio, it automatically parses the YAML file using the @stoplight/yaml package. So if you would use Stoplight Studio in combination with openapi-format, there were always changes to items were not touched (because of the parsing differences). This could lead to merge conflicts and human-error because it might not be clear anymore what was now actually changes.

skarzi commented 1 year ago

Thanks for providing more details!

I will experiment with https://github.com/eemeli/yaml and give you feedback so we can decide what to do next.

My new idea is to allow users to choose the YAML parsing engine/lib via some CLI flag, but before it is nice to test eemeli/yaml.

skarzi commented 1 year ago

I have checked https://github.com/eemeli/yaml it seems to work pretty differently than https://github.com/stoplightio/yaml, the most significant differences I have spotted are:

Appending --- to YAML with eemli/yaml is pretty straightforward, it's enough to add directives: true to stringify options. But dumping comments together with the entire content is complicated because it requires using the Document class to iterate and manipulate the YAML content.

thim81 commented 1 year ago

@skarzi Thanks for taking the time to do the investigation and reporting your findings.

Looking at the outcome, what is your conclusion on the Stoplight YAML vs Eemeli YAML? It feels like the benefits of switching are rather low, so my feeling leans towards keeping as-is? But I'm curious about your opinion.

skarzi commented 1 year ago

API descriptions are source code (they are parsed by some tools e.g. to generate HTML docs or run the mock servers as well as read/edited by humans), so I think that features like preserving comments are fundamental because in most cases humans are working with the API descriptions directly (in it raw YAML form), however, the implementation cost is pretty high because we probably need to implement some proxy object that will translate all JS Object operation into proper Document ones, so in the end, keeping as-is i is probably the better option, but it will be nice to get feedback also from other people ;)

thim81 commented 1 year ago

Hi @skarzi

I agree with your comment. It would be expected behaviour to keep comments, but like your findings I ran into the result that YAML parsing and keeping comments are not so straightforward. openapi-format has as goal to format the OpenAPI document, so in that sense removing "internal comments" is a positive side-effect. It would be nice to support the keeping of comments & --- in the future, so lets keep this issue open. I'll modify the title to broaden the scope of the item a bit.

If anybody has found a straightforward YAML parser, that can preserve comments & ---, feel free to share your input in the item.