search

flyx / NimYAML

YAML implementation for Nim
https://nimyaml.org
Other
186 stars 36 forks source link

Implement Sane Defaults #135

Closed theAkito closed 1 year ago

theAkito commented 1 year ago

Spent like half an hour tweaking the presentation options to get a fairly normal YAML. The default is pretty messed up with all this tag spam, etc. Why is that the default?

The following should be the default, I think.

  config.dump(
    fStream,
    tagStyle = tsNone,
    options = defineOptions(outputVersion = ovNone),
    handles = @[]
  )

Please, also add an option to omit ---, as this is commonly used, when chaining YAML documents, otherwise you usually don't use it for a file with only a single document inside it.

It'd be also great, if no trailing spaces were added.

Additional Context

Had to investigate & find https://github.com/flyx/NimYAML/issues/95 to be able to remove the last remaining spam line.

Good points pointed out here.

flyx commented 1 year ago

The current defaults came to be because I decided to implement YAML according to the spec, ignoring common use-cases and other implementations.

YAML was originally designed to allow different applications to communicate data. Nowadays, YAML is mostly used as configuration language that is only read (and sometimes written) by a single application, in which case most of its more complex features are unnecessary.

Since the defaults of NimYAML were designed for the original use-case, they seem unnecessarily complex in typical scenarios. That being said, NimYAML has been out and used for quite some time, and changing defaults now may break other people's code, so I'd rather not do it.

Omitting the --- is certainly missing as a presentation option, that can be added. I am also aware of the trailing whitespace problem.

As for your other suggestions, maybe a function simpleDump can be added that uses your proposed defaults.

flyx commented 1 year ago

Whitespace and --- have been addressed. I did some other improvements to the presenter while I was at it.

Concerning the defaults, I decided to do a new major version, NimYAML 2.0.0, to be able to introduce breaking changes. I am more comfortable releasing the presenter changes with such a version, since the changes I made will cause existing code emit different YAML and I'm not confident that this will cause no problems. This version will then also contain the changes to the defaults you requested.

I will start working on the new version once a final Nim 2.0.0 version is released, since that will probably need some more adjustments.

theAkito commented 1 year ago

Whitespace and --- have been addressed. I did some other improvements to the presenter while I was at it.

Concerning the defaults, I decided to do a new major version, NimYAML 2.0.0, to be able to introduce breaking changes. I am more comfortable releasing the presenter changes with such a version, since the changes I made will cause existing code emit different YAML and I'm not confident that this will cause no problems. This version will then also contain the changes to the defaults you requested.

I will start working on the new version once a final Nim 2.0.0 version is released, since that will probably need some more adjustments.

Sounds great!! 🙂

I was planning to provide some simpleDump Pull Request. Does that still make sense or would it be a waste of time, as of now? 🤔

flyx commented 1 year ago

I don't think a PR would make sense. Seeing that breaking changes are possible, I possibly want to remodel the dumping API to have a Dumper object which stores the arguments to dump since it's somewhat awkward giving those every time, and having an object makes it easier to add features in the future.

flyx commented 1 year ago

This is now fully be implemented and will be part of NimYAML 2.0.0. Release should be soon, I only need to write up migration docs as I broke some APIs.