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.

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

fix(fmt): Preserve multiple newlines between elements (#374) #919

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion parser/v2/elementparser.go
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ func addTrailingSpaceAndValidate(start parse.Position, e Element, pi *parse.Inpu
if err != nil {
return e, false, err
}
e.TrailingSpace, err = NewTrailingSpace(ws)
e.TrailingSpace, err = NewTrailingSpace(ws, true)
if err != nil {
return e, false, err
}
Expand Down
37 changes: 37 additions & 0 deletions parser/v2/elementparser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1538,6 +1538,43 @@ func TestElementParser(t *testing.T) {
},
},
},
{
name: "element: with multiple newlines, should collapse to two",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: "element: with multiple newlines, should collapse to two",
name: "element: multiple trailing newlines are collapsed to two",

input: `<div>
<span></span>



<span></span>
</div>`,
expected: Element{
Name: "div",
NameRange: Range{
From: Position{Index: 1, Line: 0, Col: 1},
To: Position{Index: 4, Line: 0, Col: 4},
},
IndentChildren: true,
Children: []Node{
Whitespace{Value: "\n\t"},
Element{
Name: "span",
NameRange: Range{
From: Position{Index: 8, Line: 1, Col: 2},
To: Position{Index: 12, Line: 1, Col: 6},
},
TrailingSpace: SpaceVerticalDouble,
},
Element{
Name: "span",
NameRange: Range{
From: Position{Index: 26, Line: 5, Col: 2},
To: Position{Index: 30, Line: 5, Col: 6},
},
TrailingSpace: SpaceVertical,
},
},
},
},
{
name: "element: can contain text that starts with for",
input: `<div>for which any
Expand Down
28 changes: 28 additions & 0 deletions parser/v2/formattestdata/element_double_newline_is_preserved.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
-- in --
package main

templ x() {
<div>
<span>Hello

World
</span>



<span>Foo Bar </span>
</div>
}
-- out --
package main

templ x() {
<div>
<span>
Hello
World
</span>

<span>Foo Bar </span>
</div>
}
2 changes: 1 addition & 1 deletion parser/v2/gocodeparser.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ var goCode = parse.Func(func(pi *parse.Input) (n Node, ok bool, err error) {
if err != nil {
return r, false, err
}
r.TrailingSpace, err = NewTrailingSpace(ws)
r.TrailingSpace, err = NewTrailingSpace(ws, true)
if err != nil {
return r, false, err
}
Expand Down
2 changes: 1 addition & 1 deletion parser/v2/stringexpressionparser.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var stringExpression = parse.Func(func(pi *parse.Input) (n Node, ok bool, err er
if err != nil {
return r, false, err
}
r.TrailingSpace, err = NewTrailingSpace(ws)
r.TrailingSpace, err = NewTrailingSpace(ws, false)
if err != nil {
return r, false, err
}
Expand Down
2 changes: 1 addition & 1 deletion parser/v2/textparser.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ var textParser = parse.Func(func(pi *parse.Input) (n Node, ok bool, err error) {
if err != nil {
return t, false, err
}
t.TrailingSpace, err = NewTrailingSpace(ws)
t.TrailingSpace, err = NewTrailingSpace(ws, false)
if err != nil {
return t, false, err
}
Expand Down
16 changes: 14 additions & 2 deletions parser/v2/textparser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func TestTextParser(t *testing.T) {
},
},
{
name: "Multiline text is colected line by line",
name: "Multiline text is collected line by line",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the typos!

input: "Line 1\nLine 2",
expected: Text{
Value: "Line 1",
Expand All @@ -92,7 +92,7 @@ func TestTextParser(t *testing.T) {
},
},
{
name: "Multiline text is colected line by line (Windows)",
name: "Multiline text is collected line by line (Windows)",
input: "Line 1\r\nLine 2",
expected: Text{
Value: "Line 1",
Expand All @@ -103,6 +103,18 @@ func TestTextParser(t *testing.T) {
TrailingSpace: "\n",
},
},
{
name: "Multiline text with multiple newlines is collected line by line",
input: "Line 1\n\n\n\nLine 2",
expected: Text{
Value: "Line 1",
Range: Range{
From: Position{Index: 0, Line: 0, Col: 0},
To: Position{Index: 6, Line: 0, Col: 6},
},
TrailingSpace: "\n",
},
},
}
for _, tt := range tests {
tt := tt
Expand Down
24 changes: 18 additions & 6 deletions parser/v2/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,17 +375,27 @@ func (t HTMLTemplate) Write(w io.Writer, indent int) error {
type TrailingSpace string

const (
SpaceNone TrailingSpace = ""
SpaceHorizontal TrailingSpace = " "
SpaceVertical TrailingSpace = "\n"
SpaceNone TrailingSpace = ""
SpaceHorizontal TrailingSpace = " "
SpaceVertical TrailingSpace = "\n"
SpaceVerticalDouble TrailingSpace = "\n\n"
)

var ErrNonSpaceCharacter = errors.New("non space character found")

func NewTrailingSpace(s string) (ts TrailingSpace, err error) {
func NewTrailingSpace(s string, allowMulti bool) (ts TrailingSpace, err error) {
joerdav marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other conversation, the bool flag makes it harder to understand what's going on without referring to the function definition. Would prefer an alternative solution, e.g. an enum style for allow multi, or functional params.

var hasHorizontalSpace bool
for _, r := range s {

runes := []rune(s)

for i, r := range s {
if r == '\n' {
if allowMulti && i < len(runes)-1 {
next := runes[i+1]
if next == '\n' {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect Windows users would like a check for \r\n.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had assumed this was normalized before entering this part of the code. Seems dangerous to have 2 code-paths based on OS everywhere in the code. I'd check, but it'd be difficult for me to test it since I don't have Windows 😅

return SpaceVerticalDouble, nil
}
}
return SpaceVertical, nil
}
if unicode.IsSpace(r) {
Expand Down Expand Up @@ -621,7 +631,9 @@ func writeNodes(w io.Writer, level int, nodes []Node, indent bool) error {
}
// Put a newline after the last node in indentation mode.
if indent && ((nextNodeIsBlock(nodes, i) || i == len(nodes)-1) || shouldAlwaysBreakAfter(nodes[i])) {
trailing = SpaceVertical
if !strings.Contains(string(trailing), string(SpaceVertical)) {
trailing = SpaceVertical
}
}
switch trailing {
case SpaceNone:
Expand Down