-
-
Notifications
You must be signed in to change notification settings - Fork 285
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
e398947
2f6aa04
dc3c684
aa31f93
24ba4f1
af46dd5
5e7bd2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -528,6 +528,8 @@ func (g *generator) writeNodes(indentLevel int, nodes []parser.Node, next parser | |
} | ||
|
||
func (g *generator) writeNode(indentLevel int, current parser.Node, next parser.Node) (err error) { | ||
maybeWhitespace := true | ||
|
||
switch n := current.(type) { | ||
case parser.DocType: | ||
err = g.writeDocType(indentLevel, n) | ||
|
@@ -540,14 +542,17 @@ func (g *generator) writeNode(indentLevel int, current parser.Node, next parser. | |
case parser.RawElement: | ||
err = g.writeRawElement(indentLevel, n) | ||
case parser.ForExpression: | ||
maybeWhitespace = false | ||
err = g.writeForExpression(indentLevel, n, next) | ||
case parser.CallTemplateExpression: | ||
err = g.writeCallTemplateExpression(indentLevel, n) | ||
case parser.TemplElementExpression: | ||
err = g.writeTemplElementExpression(indentLevel, n) | ||
case parser.IfExpression: | ||
maybeWhitespace = false | ||
err = g.writeIfExpression(indentLevel, n, next) | ||
case parser.SwitchExpression: | ||
maybeWhitespace = false | ||
err = g.writeSwitchExpression(indentLevel, n, next) | ||
case parser.StringExpression: | ||
err = g.writeStringExpression(indentLevel, n.Expression) | ||
|
@@ -566,8 +571,9 @@ func (g *generator) writeNode(indentLevel int, current parser.Node, next parser. | |
// Write trailing whitespace, if there is a next node that might need the space. | ||
// If the next node is inline or text, we might need it. | ||
// If the current node is a block element, we don't need it. | ||
needed := (isInlineOrText(current) && isInlineOrText(next)) | ||
if ws, ok := current.(parser.WhitespaceTrailer); ok && needed { | ||
// If, switch and for as current node skip whitespace, but not always when next node. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this comment either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If, switch and for statements are marked as inline elements. I don't agree with this, but I'm sure there are reasons for this. The thing is, when these statements got their own TrailingSpace, they no longer satisfy the tests which were written to handle "2 inline elements following each other", which the previous code did here. These statements should not add a whitespace if they're the current node, but if they're the "next" node (and the current is inline), they should add a whitespace. At least, that's what the failing tests indicated to me. Fixing the issue in any other way caused other tests to fail, indicating some weird inconsistency that was only solved via this edge-case. Again, as mentioned in my previous comment, I'm not happy with any of these solutions and if you have a better plan for this, feel free. These were really the last pieces of failing tests that I tried to fix without making modifications like this, but were unsuccessful. |
||
neededWhitespace := maybeWhitespace && isInlineOrText(current) && isInlineOrText(next) | ||
if ws, ok := current.(parser.WhitespaceTrailer); ok && neededWhitespace { | ||
if err := g.writeWhitespaceTrailer(indentLevel, ws.Trailing()); err != nil { | ||
return err | ||
} | ||
|
@@ -604,7 +610,7 @@ func (g *generator) writeWhitespaceTrailer(indentLevel int, n parser.TrailingSpa | |
} | ||
// Normalize whitespace for minified output. In HTML, a single space is equivalent to | ||
// any number of spaces, tabs, or newlines. | ||
if n == parser.SpaceVertical { | ||
if n == parser.SpaceVertical || n == parser.SpaceVerticalDouble { | ||
n = parser.SpaceHorizontal | ||
} | ||
if _, err = g.w.WriteStringLiteral(indentLevel, string(n)); err != nil { | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,5 +29,11 @@ func (p callTemplateExpressionParser) Parse(pi *parse.Input) (n Node, ok bool, e | |
return | ||
} | ||
|
||
// Parse trailing whitespace. | ||
r.TrailingSpace, err = parseTrailingSpace(pi, true, false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not ideal that from reading the file, I have no idea what the A better design would use an enum to define the behaviour, e.g. something like this: type TrailingSpaceParseOpts int
const (
ParseTrailingAllowVerticalAndHorizontal TrailingSpaceParseOpts = iota
ParseTrailingAllowVerticalOnly
)
parseTrailingSpace(pi, ParseTrailingAllowVerticalAndHorizontal) That way, it's obvious what the intent of the code is. |
||
if err != nil { | ||
return r, false, err | ||
} | ||
|
||
return r, true, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,28 @@ func TestCallTemplateExpressionParser(t *testing.T) { | |
}, | ||
}, | ||
}, | ||
{ | ||
name: "call: can parse the initial expression and leave the text", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! |
||
input: `{!Other(p.Test)} Home`, | ||
expected: CallTemplateExpression{ | ||
Expression: Expression{ | ||
Value: "Other(p.Test)", | ||
Range: Range{ | ||
From: Position{ | ||
Index: 2, | ||
Line: 0, | ||
Col: 2, | ||
}, | ||
To: Position{ | ||
Index: 15, | ||
Line: 0, | ||
Col: 15, | ||
}, | ||
}, | ||
}, | ||
TrailingSpace: SpaceHorizontal, | ||
}, | ||
}, | ||
} | ||
for _, tt := range tests { | ||
tt := tt | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1538,6 +1538,43 @@ func TestElementParser(t *testing.T) { | |||||
}, | ||||||
}, | ||||||
}, | ||||||
{ | ||||||
name: "element: with multiple newlines, should collapse to two", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,7 +10,10 @@ var forExpression parse.Parser[Node] = forExpressionParser{} | |||||
type forExpressionParser struct{} | ||||||
|
||||||
func (forExpressionParser) Parse(pi *parse.Input) (n Node, ok bool, err error) { | ||||||
var r ForExpression | ||||||
r := ForExpression{ | ||||||
// Default behavior is always a trailing space | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
In templ, comments are always complete sentences, terminating with a full stop. |
||||||
TrailingSpace: SpaceVertical, | ||||||
} | ||||||
start := pi.Index() | ||||||
|
||||||
// Strip leading whitespace and look for `for `. | ||||||
|
@@ -48,5 +51,11 @@ func (forExpressionParser) Parse(pi *parse.Input) (n Node, ok bool, err error) { | |||||
return | ||||||
} | ||||||
|
||||||
// Parse trailing whitespace. | ||||||
r.TrailingSpace, err = parseTrailingSpace(pi, true, true) | ||||||
if err != nil { | ||||||
return r, false, err | ||||||
} | ||||||
|
||||||
return r, true, nil | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
-- in -- | ||
package main | ||
|
||
templ test() { | ||
<span></span> | ||
|
||
|
||
{! Other(p.Test) } | ||
{!Other(p.Test)} Home | ||
|
||
<div>Some standard templ</div> | ||
|
||
{!Other(p.Test) } | ||
|
||
|
||
|
||
<span></span> | ||
|
||
} | ||
-- out -- | ||
package main | ||
|
||
templ test() { | ||
<span></span> | ||
|
||
@Other(p.Test) | ||
@Other(p.Test) Home | ||
<div>Some standard templ</div> | ||
|
||
@Other(p.Test) | ||
|
||
<span></span> | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
-- in -- | ||
package main | ||
|
||
templ test() { | ||
<!-- This is a comment --> | ||
|
||
// This is not included in the output. | ||
<div>Some standard templ</div> | ||
|
||
|
||
/* This is not included in the output too. */ | ||
/* | ||
Leave this alone. | ||
*/ | ||
|
||
|
||
} | ||
-- out -- | ||
package main | ||
|
||
templ test() { | ||
<!-- This is a comment --> | ||
|
||
// This is not included in the output. | ||
<div>Some standard templ</div> | ||
|
||
/* This is not included in the output too. */ | ||
/* | ||
Leave this alone. | ||
*/ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
-- in -- | ||
package main | ||
|
||
templ x() { | ||
<div> | ||
@Hero() | ||
<span></span> | ||
|
||
|
||
@Hero() | ||
|
||
|
||
|
||
<span></span> | ||
</div> | ||
} | ||
-- out -- | ||
package main | ||
|
||
templ x() { | ||
<div> | ||
@Hero() | ||
<span></span> | ||
|
||
@Hero() | ||
|
||
<span></span> | ||
</div> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
-- in -- | ||
package main | ||
|
||
templ x() { | ||
<div> | ||
<a></a><span></span> | ||
|
||
// This line is indented incorrectly | ||
<a></a> | ||
|
||
</div> | ||
} | ||
|
||
-- out -- | ||
package main | ||
|
||
templ x() { | ||
<div> | ||
<a></a><span></span> | ||
|
||
// This line is indented incorrectly | ||
<a></a> | ||
|
||
</div> | ||
} |
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> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
-- in -- | ||
package main | ||
|
||
// The below template caused the generated code to not strip newlines, causing error | ||
templ x() { | ||
<div> | ||
<a></a> | ||
|
||
|
||
|
||
<a> | ||
</a> | ||
</div> | ||
} | ||
-- out -- | ||
package main | ||
|
||
// The below template caused the generated code to not strip newlines, causing error | ||
templ x() { | ||
<div> | ||
<a></a> | ||
|
||
<a></a> | ||
</div> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be whitespace
implies that something might be whitespace, but with this variable, I think you're trying to say that trailing whitespace is not be allowed if this set to true, therefore, a better variable name would beisTrailingWhitespaceAllowed
.And
forceWhitespace
might be better off namedforceTrailingWhitespace
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is the opposite; we if this is true, we might add a whitespace. If it's false, we definitely won't add a whitespace. It's only there to prevent If, For and Switch expressions from adding a whitespace, since that's handled elsewhere.
But I agree, your variable names are better.