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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e398947
fix(fmt): Preserve multiple newlines between elements (#374)
AlexanderArvidsson Sep 12, 2024
2f6aa04
fix(fmt): Double newline indent & strip newline in minify
AlexanderArvidsson Sep 13, 2024
dc3c684
fix(fmt): Preserve newline after templ elements
AlexanderArvidsson Sep 13, 2024
aa31f93
fix(fmt): Preserve newlines in most parsers (#374)
AlexanderArvidsson Sep 19, 2024
24ba4f1
fix(fmt): Simplify setting trailing space (#374)
AlexanderArvidsson Sep 20, 2024
af46dd5
fix(gen): If, for, switch test whitespace (#374)
AlexanderArvidsson Sep 29, 2024
5e7bd2e
fix(gen): TemplElementExpression whitespace test (#374)
AlexanderArvidsson Sep 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions generator/generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,9 @@ 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
Copy link
Owner

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 be isTrailingWhitespaceAllowed.

And forceWhitespace might be better off named forceTrailingWhitespace.

Copy link
Author

@AlexanderArvidsson AlexanderArvidsson Sep 30, 2024 •
edited
Loading

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.

forceWhitespace := false

switch n := current.(type) {
case parser.DocType:
err = g.writeDocType(indentLevel, n)
Expand All @@ -540,14 +543,25 @@ 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)

// TemplElementExpression with block should always have whitespace if the next element is also
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand this comment. It doesn't make grammatical sense.

  • TemplElementExpression with block... block what? Are we saying that a templ element expression that is a block HTML element the it should always have following whitespace?
  • "if the next element is also..." what? What is the next element?

The code doesn't check whether the element is a block element, from what I can see it checks whether the current element has children, AND that the next child is also a templ element.

An element can have children, and still be an inline element, e.g. <i>child text node</i>. I haven't tested it out, but if the comment is accurate, with <i>child text node</i><b>next node</b>, we might end up with a line break between those inline nodes, since whitespace would be "forced".

There's also no nil check here on next. I haven't checked the call sites, but it looks like next could be nil, and this might panic if there's an node with children at the end of a list of nodes.

Suggest explaining the expected behaviour (why of the comment), adding a nil check, and adding a test to make sure that subsequent inline elements don't inappropriately get space between them.

Copy link
Author

@AlexanderArvidsson AlexanderArvidsson Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Great feedback! Let me explain:

The full comment on that line is:

		// TemplElementExpression with block should always have whitespace if the next element is also
		// a TemplElementExpression

I wrapped it to keep within a sensible line width. Upon reading your other comments I see your convention seems to have 1 sentence per line, with a period.

Templ elements can contain children, which is then a Go block (not HTML block), and is called as such in the code base in tests. I merely used the same terminology, but I see the confusion with HTML blocks.

		{
			name: "templelement: simple, block with text",
			input: `@Other(p.Test) {
	some words
}`,

When I didn't add this special case, tests were failing. See my comment in one of the outdated threads.
There are tests that expect horizontal spacing between 2 TemplElementExpressions that contain children.
See test test-import, which includes multiple Templ elements.
This test already tests this case (albeit without a self-explanatory name), so not necessary to add another one. I'd argue to rename it, since I don't even understand what its testing, there are no imports in the test.

The nil check was my bad, missed that one, sorry.

Copy link
Author

@AlexanderArvidsson AlexanderArvidsson Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if the comment is accurate, with child text nodenext node, we might end up with a line break between those inline nodes, since whitespace would be "forced".

Let me clarify that what the failing tests were checking for here is a space, not a newline.
I think you're right though that this change could be adding a line break. Let me add some tests for this.

// a TemplElementExpression
if len(n.Children) > 0 {
if _, ok := next.(parser.TemplElementExpression); ok {
forceWhitespace = true
}
}
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)
Expand All @@ -566,8 +580,10 @@ 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.
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand this comment either.

Copy link
Author

@AlexanderArvidsson AlexanderArvidsson Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 := forceWhitespace || (maybeWhitespace && isInlineOrText(current) && isInlineOrText(next))
Copy link
Owner

Choose a reason for hiding this comment

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

neededWhitespace puts the variable into past tense - i.e. that we needed whitespace in the past.

However, I now see that the algorithm for deciding whether to add trailing whitespace is mixed in with the logic for writing out various nodes, plus this final statement.

I think it would be clearer if this was a function called, e.g. shouldAllowTrailingWhitespace(current, next) and the logic applied there, e.g. something that looks like this (but not this!):

func shouldAllowTrailingWhitespace(current, next) bool {
   if _, isTemplElement := parser.TemplElementExpression; isTemplElement {
		if len(n.Children) > 0 {
			if _, ok := next.(parser.TemplElementExpression); ok {
				return true
		}
	}
   }
   switch n := current.(type) {
      case x:
        return false
      case y:
         return false     
   }
   
}

That way, the logic for testing trailing whitespace can be explained more easily with some tests.

Copy link
Author

@AlexanderArvidsson AlexanderArvidsson Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, this whole function was extremely confusion. I tried to do the absolute minimal changes to it to allow the existing tests to pass. While I am not happy with it, the truth is that this function needs a huge refactoring. It generalizes adding whitespaces after elements, but I don't think that its something that can be generalized like this.

I probably don't have the full knowledge of the codebase to write a better one, or argue why a different solution is better. Your function makes it much clearer though, so that is a good start.


if ws, ok := current.(parser.WhitespaceTrailer); ok && neededWhitespace {
if err := g.writeWhitespaceTrailer(indentLevel, ws.Trailing()); err != nil {
return err
}
Expand Down Expand Up @@ -604,7 +620,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 {
Expand Down
6 changes: 6 additions & 0 deletions parser/v2/calltemplateparser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The 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 true and false arguments mean. I'd have to find parseTrailingSpace and read the args.

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
}
22 changes: 22 additions & 0 deletions parser/v2/calltemplateparser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,28 @@ func TestCallTemplateExpressionParser(t *testing.T) {
},
},
},
{
name: "call: can parse the initial expression and leave the text",
Copy link
Owner

Choose a reason for hiding this comment

The 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
Expand Down
6 changes: 1 addition & 5 deletions parser/v2/elementparser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,11 +467,7 @@ func addTrailingSpaceAndValidate(start parse.Position, e Element, pi *parse.Inpu
return e, false, err
}
// Add trailing space.
ws, _, err := parse.Whitespace.Parse(pi)
if err != nil {
return e, false, err
}
e.TrailingSpace, err = NewTrailingSpace(ws)
e.TrailingSpace, err = parseTrailingSpace(pi, true, false)
if err != nil {
return e, false, err
}
Expand Down
37 changes: 37 additions & 0 deletions parser/v2/elementparser_test.go
View file
Open in desktop
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
11 changes: 10 additions & 1 deletion parser/v2/forexpressionparser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
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
// Default behavior is always a trailing space
// Default behavior is always a trailing space.

In templ, comments are always complete sentences, terminating with a full stop.

TrailingSpace: SpaceVertical,
}
start := pi.Index()

// Strip leading whitespace and look for `for `.
Expand Down Expand Up @@ -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
}
2 changes: 2 additions & 0 deletions parser/v2/forexpressionparser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ func TestForExpressionParser(t *testing.T) {
TrailingSpace: SpaceVertical,
},
},
TrailingSpace: SpaceVertical,
},
},
{
Expand Down Expand Up @@ -117,6 +118,7 @@ func TestForExpressionParser(t *testing.T) {
TrailingSpace: SpaceVertical,
},
},
TrailingSpace: SpaceVertical,
},
},
}
Expand Down
34 changes: 34 additions & 0 deletions parser/v2/formattestdata/calltemplate_newline_is_preserved.txt
View file
Open in desktop
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>

}
31 changes: 31 additions & 0 deletions parser/v2/formattestdata/comments_newline_is_preserved.txt
View file
Open in desktop
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.
*/
}
29 changes: 29 additions & 0 deletions parser/v2/formattestdata/element_double_newline_after_component.txt
View file
Open in desktop
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>
}
25 changes: 25 additions & 0 deletions parser/v2/formattestdata/element_double_newline_indent_issue.txt
View file
Open in desktop
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>
}
28 changes: 28 additions & 0 deletions parser/v2/formattestdata/element_double_newline_is_preserved.txt
View file
Open in desktop
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>
}
25 changes: 25 additions & 0 deletions parser/v2/formattestdata/element_double_newline_string_literal_termination_issue.txt
View file
Open in desktop
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>
}
3 changes: 2 additions & 1 deletion parser/v2/formattestdata/for_loops_are_placed_on_a_new_line.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ package test
templ input(items []string) {
<div>{ "the" }<div>{ "other" }</div>for _, item := range items {
<div>{ item }</div>
}</div>
}<span>After closing bracket</span></div>
}
-- out --
package test
Expand All @@ -16,5 +16,6 @@ templ input(items []string) {
for _, item := range items {
<div>{ item }</div>
}
<span>After closing bracket</span>
</div>
}
Loading