Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cue/format: let/comprehension incorrectly indented when first expression in file #1544

Closed
myitcv opened this issue Feb 22, 2022 · 0 comments
Closed

cue/format: let/comprehension incorrectly indented when first expression in file #1544

myitcv opened this issue Feb 22, 2022 · 0 comments
Assignees
@mvdan
Labels
fmt Related to formatting functionality. NeedsFix

Comments

@myitcv
Copy link
Member

myitcv commented Feb 22, 2022 •
edited
Loading

What version of CUE are you using (cue version)?

$ cue version
cue version +a629625c linux/arm64

Does this issue reproduce with the latest release?

Yes

What did you do?

cp x.cue x.cue.golden
cp y.cue y.cue.golden
cp z.cue z.cue.golden

exec cue fmt x.cue y.cue z.cue

cmp x.cue x.cue.golden
cmp y.cue y.cue.golden
cmp z.cue z.cue.golden

-- x.cue --
if true {
	x: 5
}
-- y.cue --
let x = 5
y: x
-- z.cue --
for v in ["a", "b"] {
	(v): v
}

What did you expect to see?

Passing test

What did you see instead?

> cp x.cue x.cue.golden
> cp y.cue y.cue.golden
> cp z.cue z.cue.golden
> exec cue fmt x.cue y.cue z.cue
> cmp x.cue x.cue.golden
--- x.cue
+++ x.cue.golden
@@ -1,3 +1,3 @@
- if true {
+if true {
        x: 5
 }

FAIL: /tmp/testscript1129945673/repro.txt/script.txt:7: x.cue and x.cue.golden differ
error running repro.txt in /tmp/testscript1129945673/repro.txt

But note that currently all comparisons fail because in each case the expression is indented by a single space.
@myitcv myitcv added NeedsFix fmt Related to formatting functionality. labels Feb 22, 2022
@myitcv myitcv changed the title cue/format: let incorrectly indented when first expression in file cue/format: let/comprehension incorrectly indented when first expression in file Feb 22, 2022
FogDong added a commit to FogDong/cue that referenced this issue May 6, 2022
@FogDong
cue/format/printer: fix indentation for clauses
681841d 
This PR reset the allow for clauses at the position 0.

Fixes cue-lang#1544

Signed-off-by: Fog Dong <wuwuglu19@gmail.com>
@FogDong FogDong mentioned this issue May 6, 2022
Closed
FogDong added a commit to FogDong/cue that referenced this issue May 10, 2022
@FogDong
cue/format/printer: fix indentation for clauses
b73a9d0 
This PR reset the allow of blanks at the position 0.

Fixes cue-lang#1544

Signed-off-by: Fog Dong <wuwuglu19@gmail.com>
FogDong added a commit to FogDong/cue that referenced this issue May 10, 2022
@FogDong
cue/format/printer: fix indentation for clauses
b359835 
This PR reset the allow for blanks at the position 0.

Fixes cue-lang#1544

Signed-off-by: Fog Dong <wuwuglu19@gmail.com>
cueckoo pushed a commit that referenced this issue May 24, 2022
@mvdan
cue/format: omit whitespace before the first clause
2b7d5be 
When printing clauses, we often want to print a whitespace before the
token "let", "if", and "for". For example:

	for i, v in src if i > 1 {

As opposed to omitting spaces, which breaks the syntax:

	for i, v in srcif i > 1 {

However, we also printed a space before the first clause in the list.
This could result in a CUE file beginning with a space if the first
token is a clause:

	$ cue fmt .
	$ cat if.cue
	 if foo {
	}

First, we fix that bug for clauses by not having walkClauseList call
f.print(blank) before the first clause.

Second, we also fix it for "let" declarations by teaching mayCombine
about the previous token being ILLEGAL - meaning that we're printing the
first token in the entire file.

It is worth noting that this also changes how we print the first clause
inside another expression; for instance:

	cfgs: [ for crd in ["one", "two"] {
	}]

Now formats without the leading space, just like at the beginning of a file:

	cfgs: [for crd in ["one", "two"] {
	}]

Arguably that is more consistent, as there is no space after the "for"
clause, following the "}" token.

Fixes #1544.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I72e4618f2b1011005316c617a32e0c3a6ff3b706
@myitcv myitcv added this to the fmt-redesign milestone Apr 27, 2023
@myitcv myitcv added the zGarden label Jun 13, 2023
@mvdan mvdan mentioned this issue Jun 26, 2023
Closed
@jpluscplusm jpluscplusm mentioned this issue Aug 18, 2023
Closed
@mvdan mvdan removed the zGarden label Feb 8, 2024
@mvdan mvdan removed this from the fmt-redesign milestone Mar 19, 2024
@mvdan mvdan self-assigned this May 1, 2024
cueckoo pushed a commit that referenced this issue Jun 10, 2024
@mvdan
cue/format: rewrite TestFiles with testscript
45720ed 
This allows us to test many small files without exploding the number
of files tracked by VCS inside testdata.
For example, to fix https://cuelang.org/issue/1544,
we would want to cover various clauses at the start of a short file.

For #1544.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: Ifd93b98c26028de9e03f33ea697af49047989b61
cueckoo pushed a commit that referenced this issue Jun 10, 2024
@mvdan
cue/format: add a regression test for issue 1544
67e6122 
This shows the current buggy behavior, where comprehensions at the start
of a file gain a blank space.

For #1544.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I9751f0eae5408c2c9e11b95e83107441e3209cc4
cueckoo pushed a commit that referenced this issue Jun 10, 2024
@mvdan
cue/format: omit whitespace before the first clause
80ed407 
When printing clauses, we often want to print a whitespace before the
token "let", "if", and "for". For example:

	for i, v in src if i > 1 {

As opposed to omitting spaces, which breaks the syntax:

	for i, v in srcif i > 1 {

However, we also printed a space before the first clause in the list.
This could result in a CUE file beginning with a space if the first
token is a clause:

	$ cue fmt .
	$ cat if.cue
	 if foo {
	}

First, we fix that bug for clauses by not having walkClauseList call
f.print(blank) before the first clause.

Second, we also fix it for "let" declarations by teaching mayCombine
about the previous token being ILLEGAL - meaning that we're printing the
first token in the entire file.

It is worth noting that this also changes how we print the first clause
inside another expression; for instance:

	cfgs: [ for crd in ["one", "two"] {
	}]

Now formats without the leading space, just like at the beginning of a file:

	cfgs: [for crd in ["one", "two"] {
	}]

Arguably that is more consistent, as there is no space after the "for"
clause, following the "}" token.

Fixes #1544.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I72e4618f2b1011005316c617a32e0c3a6ff3b706
cueckoo pushed a commit that referenced this issue Jun 10, 2024
@mvdan
cue/format: rewrite TestFiles with testscript
08b1ce0 
This allows us to test many small files without exploding the number
of files tracked by VCS inside testdata.
For example, to fix https://cuelang.org/issue/1544,
we would want to cover various clauses at the start of a short file.

For #1544.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: Ifd93b98c26028de9e03f33ea697af49047989b61
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1196106
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
cueckoo pushed a commit that referenced this issue Jun 10, 2024
@mvdan
cue/format: add a regression test for issue 1544
475f692 
This shows the current buggy behavior, where comprehensions at the start
of a file gain a blank space.

For #1544.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: I9751f0eae5408c2c9e11b95e83107441e3209cc4
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1196107
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mvdan mvdan

Labels
fmt Related to formatting functionality. NeedsFix
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cue/format/printer: fix indentation for clauses FogDong/cue
2 participants
@myitcv @mvdan