google / yamlfmt

An extensible command line tool or library to format yaml files.
Apache License 2.0
1.18k stars 44 forks source link

Multiline syntax with pipe (`|`) not retained #185

Open tamird opened 5 months ago

tamird commented 5 months ago

I see this diff:

     steps:
       - name: Start PostgreSQL
         # We start PostgreSQL as a docker step instead of the GH `services` because it allows us to pass in custom flags (and seems to be faster to spin up?)
-        run: |
-          set -euo pipefail
-          args=(
-            -d
-            --name postgres 
-            --env POSTGRES_USER=postgres 
-            --env POSTGRES_PASSWORD=postgres 
-            --publish 5432:5432 
-            --health-cmd pg_isready 
-            --health-interval 10s 
-            --health-timeout 5s 
-            --health-retries 5 
-            public.ecr.aws/docker/library/postgres:16-alpine 
-
-            # This is to allow many connections from xdist - we run as many DBs in parallel as CPUs on the box
-            -c max_locks_per_transaction=1000 
-            -c max_connections=1000
-          )
-          docker run "${args[@]}"
-
+        run: "set -euo pipefail\nargs=(\n  -d\n  --name postgres \n  --env POSTGRES_USER=postgres \n  --env POSTGRES_PASSWORD=postgres \n  --publish 5432:5432 \n  --health-cmd pg_isready \n  --health-interval 10s \n  --health-timeout 5s \n  --health-retries 5 \n  public.ecr.aws/docker/library/postgres:16-alpine \n      #magic___^_^___line\n  # This is to allow many connections from xdist - we run as many DBs in parallel as CPUs on the box\n  -c max_locks_per_transaction=1000 \n  -c max_connections=1000\n)\ndocker run \"${args[@]}\"\n      #magic___^_^___line\n"

This was previously discussed in https://github.com/google/yamlfmt/issues/62#issuecomment-1282779834.

braydonk commented 5 months ago

Hi @tamird thanks for opening an issue. Not sure whether this will work, but does adding scan_folded_as_literal: true fix this?

tamird commented 5 months ago

No, the behavior is unchanged.

kachick commented 5 months ago

I have the same problem and it is reproduced when an included line has whitespace just before a line feed.

yamlfmt -version

cat <<'EOF' | yamlfmt -in
a: |
 b
 c
EOF

cat <<'EOF' | yamlfmt -in
a: |
 b 
 c
EOF

This script outputs as follows.

0.12.0
a: |
  b
  c
a: "b \nc\n"

Actually used 0.12.1, 0.12.0 is the wrong displayed version from known problem


Ah, this problem looks an known issue 🙇‍♂️

seastco commented 5 months ago

seeing this as well

19ngaddam commented 5 months ago

the "fix" that worked for me was ensuring that there were no trailing spaces anywhere in the multiline block (including after the |). yamlfmt was able to preserve the format in that case. Ideally, there should be an option to ignore the format for multiline literal style strings.

braydonk commented 5 months ago

Right I remember this now. I was trying to solve this in the yaml library and got stuck.

An interim fix could be to add a feature that trims the whitespace at the end of every string; that's doable without parsing. It would probably be a relatively useful feature anyway.

My time to work on this project over the next couple weeks is unfortunately quite limited so I can't give an estimate for when that will get done, but next time I have time I will get this done.

thiagowfx commented 5 months ago

Just noting that something similar happens with >- (it's likely the same root cause).

To add another future test case idea:

Observed:

diff --git ansible/acme.sh/acme.yml ansible/acme.sh/acme.yml
index 08b5f893..1c9549be 100644
--- ansible/acme.sh/acme.yml
+++ ansible/acme.sh/acme.yml
@@ -44,12 +44,7 @@

 - name: Issue cert for domain via DNS challenge mode
   command: >-
-    ./acme.sh --issue -d {{ acme_sh_domain }}
-    --challenge-alias {{ acme_sh_challenge_domain }}
-    --dns {{ acme_sh_dns_provider }}
-    --accountemail {{ acme_sh_mail }}
-    {{ "--staging" if acme_sh_use_staging is defined  else "" }}
-    {{ "--debug" if acme_sh_debug is defined else "" }}
+    ./acme.sh --issue -d {{ acme_sh_domain }} --challenge-alias {{ acme_sh_challenge_domain }} --dns {{ acme_sh_dns_provider }} --accountemail {{ acme_sh_mail }} {{ "--staging" if acme_sh_use_staging is defined  else "" }} {{ "--debug" if acme_sh_debug is defined else "" }}
   args:
     chdir: "~/.acme.sh"
   environment:

Desired / expected: Newlines should have been preserved.

braydonk commented 4 months ago

I did add trim_trailing_whitespace as an option to the formatter. With this turned on, this bug shouldn't get tripped since it's caused by trailing whitespace. Not a perfect fix, but a stopgap at least.

nikaro commented 3 months ago

I did add trim_trailing_whitespace as an option to the formatter. With this turned on, this bug shouldn't get tripped since it's caused by trailing whitespace. Not a perfect fix, but a stopgap at least.

This does not work for with >, only with |.

# cat .yamlfmt.yaml
trim_trailing_whitespace: true
# cat hello.yaml
multiline_without_trailing_whitspaces: >-
  hello
  world
# yamlfmt -debug=all -dry hello.yaml
[DEBUG]: No config path specified in -conf
[DEBUG]: Found config at /var/folders/pz/wn2sts951hbdjx0nzdr1c_h40000gp/T/tmp.J6M98dAOmk/.yamlfmt.yaml
[DEBUG]: Found config at /var/folders/pz/wn2sts951hbdjx0nzdr1c_h40000gp/T/tmp.J6M98dAOmk/.yamlfmt.yaml
[DEBUG]: using file path matching. include patterns: [hello.yaml]
[DEBUG]: found paths: [hello.yaml]
[DEBUG]: paths to format: map[hello.yaml:{}]
hello.yaml:
  multiline_without_trailing_whitspaces: >-  multiline_without_trailing_whitspaces: >-
-   hello                                      hello world
-   world
braydonk commented 3 months ago

Hi @nikaro,

Two things:

  1. Is the example in cat .yamlfmt.yaml your exact config file? If so, that will not work. trim_trailing_whitespace goes under a formatter.
  2. Folded scalar style (>) are a separate issue. For those, you will also need to set the formatter option scan_folded_as_literal: true. This is because when the underlying YAML parser reads the YAML and reproduces it, it tries to respect the > (which means to fold all newlines and produce the string as a single line). That option circumvents that behaviour.

Applying both of those suggestions, I expect it will start to work. If it doesn't, please open a new issue with similar reproduction steps.

EDIT: I corrected my suggestion in 2

nikaro commented 3 months ago

Hi @braydonk

  1. No it wasn't my exact config, I tried to reproduce a minimal config/environment, my bad.
  2. Ah, I didn't knew this bit.

It works with your suggestion. Thanks.