iterative / dvc

🦉 ML Experiments and Data Management with Git
https://dvc.org
Apache License 2.0
13.66k stars 1.17k forks source link

yaml: serialize data with list offset size 2 #10379

Closed sisp closed 5 months ago

sisp commented 5 months ago

Just a minor esthetics change: I've updated the indentation settings for YAML serialization to align better with common formatting styles.

YAML is often formatted with a 2 spaces offset for lists. For instance, Prettier formats it like this, and dvc.yaml file examples in the DVC docs also use this formatting style.

 k:
-- 1
+  - 1
-- 2
+  - 2

Also, ruamel.yaml's documentation states:

The above example with the often seen yaml.indent(mapping=2, sequence=4, offset=2) indentation:

x:
  y:
    - b: 1
    - 2

https://yaml.readthedocs.io/en/latest/detail/#indentation-of-block-sequences


codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.71%. Comparing base (dd2b515) to head (4503a4e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #10379 +/- ## ========================================== - Coverage 90.72% 90.71% -0.02% ========================================== Files 501 501 Lines 38865 38866 +1 Branches 5618 5618 ========================================== - Hits 35262 35258 -4 - Misses 2961 2964 +3 - Partials 642 644 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dberenbaum commented 5 months ago

@sisp I personally find offset 2 more readable, but I'm hesitant to approve this for a couple reasons:

  1. I think we would rather avoid deviating from the ruamel.yaml defaults. Maybe it is something you could take up with them?
  2. I think it will be surprising if a minor update reformats dvc.yaml files.
sisp commented 5 months ago

Thanks for your feedback, @dberenbaum! :pray:

I could raise this topic upstream, but if they agree than the result for DVC will be the same – a reformatting of YAML(-like) files but potentially even unrelated to any DVC release since ruamel.yaml's version is only lower-bounded (which is good!):

https://github.com/iterative/dvc/blob/dd2b515c97a6757c6c3522329d6259e0e86592df/pyproject.toml#L67

shcheklein commented 5 months ago

I agree with Dave here. I would not change the format w/o a very strong reason (some other tool defaults probably is not strong enough imo).

Also - should it be configurable (we won't be able to serve everyone), also should DVC try to preserve the existing formatting (this is probably not easy)?

skshetry commented 5 months ago

DVC already preserves formatting of yaml files. The stylistic choices are only applied when dvc creates a new file. In that case, you are better using pre-commit hooks.

I am -1 on adding any configuration.

dberenbaum commented 5 months ago

@sisp Thanks for the PR, but I am going to close this one since the team agrees this is not something we want to handle in dvc.

sisp commented 5 months ago

Fair enough, @dberenbaum.