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

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 1 commit
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
10 changes: 8 additions & 2 deletions generator/generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
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.


switch n := current.(type) {
case parser.DocType:
err = g.writeDocType(indentLevel, n)
Expand All @@ -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)
Expand All @@ -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.
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 := maybeWhitespace && isInlineOrText(current) && isInlineOrText(next)
if ws, ok := current.(parser.WhitespaceTrailer); ok && neededWhitespace {
if err := g.writeWhitespaceTrailer(indentLevel, ws.Trailing()); err != nil {
return err
}
Expand Down
8 changes: 0 additions & 8 deletions generator/test-import/template_templ.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.