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

feat: HTML/CSS/JS ContextWriter #636

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

feat: HTML/CSS/JS ContextWriter #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e320d95
feat: add Expression to attribute/element structs
alehechka Feb 20, 2024
62382ba
Merge branch 'main' into feature/html-element-positions
alehechka Feb 21, 2024
71e3a34
add position setters and update unit tests
alehechka Feb 22, 2024
6ae9a5e
chore: fix unit tests with position expressions
alehechka Feb 23, 2024
bc5092f
fix remaining parser unit tests
alehechka Feb 24, 2024
7891bda
chore: build version
alehechka Feb 24, 2024
ee13b51
Merge branch 'main' into feature/html-element-positions
alehechka Feb 26, 2024
da94fcf
chore: remove HTMLExpression interface
alehechka Feb 26, 2024
a04c725
replace Expression with Range in all HTML types
alehechka Feb 29, 2024
e153d93
chore: get-version
alehechka Feb 29, 2024
761d675
Merge branch 'main' into feature/html-element-positions
alehechka Mar 6, 2024
9656641
chore: rename field as NameRange
alehechka Mar 6, 2024
ee298d2
Merge branch 'main' into feature/html-lsp
alehechka Mar 13, 2024
ec259fd
feat: refactor all Write methods with new ContextWriter
alehechka Mar 13, 2024
f0051c9
chore: add WriteContext as param to NewContextWriter
alehechka Mar 14, 2024
d972e09
wip: resolve broken writers and update unit tests
alehechka Mar 14, 2024
808514f
fix: ExpressionAttribute conditional writes and unit tests
alehechka Mar 15, 2024
4810026
fix: smarter white space preservation
alehechka Mar 16, 2024
0edebff
Merge branch 'main' into feature/html-lsp
alehechka Mar 16, 2024
556439a
chore: add more expextedHTML to unit tests
alehechka Mar 16, 2024
601e8f1
chore: more html contextwriter unit tests
alehechka Mar 17, 2024
59c9985
chore: css parser unit tests
alehechka Mar 21, 2024
bae606c
chore: remove unused IndexToContext
alehechka Mar 21, 2024
2d46172
Merge branch 'main' into feature/html-lsp
alehechka Mar 21, 2024
53a7446
Merge branch 'main' into feature/html-lsp
alehechka Mar 30, 2024
4c7959b
fix: add line length checking to unit tests
alehechka Mar 30, 2024
cee405d
fix: test line lengths against actual outputs
alehechka Mar 30, 2024
6de021c
chore: refactor some CW uses
alehechka Mar 30, 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
2 changes: 1 addition & 1 deletion .version
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0.2.648
0.2.651
3 changes: 2 additions & 1 deletion cmd/templ/fmtcmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ func format(write writer, read reader) (err error) {
return err
}
w := new(bytes.Buffer)
if err = t.Write(w); err != nil {
cw := parser.NewContextWriter(w, parser.WriteContextAll)
if err = t.Write(cw); err != nil {
return fmt.Errorf("formatting error: %w", err)
}
return write(fileName, w.String())
Expand Down
3 changes: 2 additions & 1 deletion cmd/templ/lspcmd/proxy/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,8 @@ func (p *Server) Formatting(ctx context.Context, params *lsp.DocumentFormattingP
return
}
w := new(strings.Builder)
err = template.Write(w)
cw := parser.NewContextWriter(w, parser.WriteContextAll)
err = template.Write(cw)
if err != nil {
p.Log.Error("handleFormatting: faled to write template", zap.Error(err))
return
Expand Down
3 changes: 2 additions & 1 deletion cmd/templ/migratecmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ func migrate(fileName string) (err error) {

// Write the updated file.
w := new(bytes.Buffer)
err = v2Template.Write(w)
cw := v2.NewContextWriter(w, v2.WriteContextAll)
err = v2Template.Write(cw)
if err != nil {
return fmt.Errorf("%s formatting error: %w", fileName, err)
}
Expand Down
106 changes: 89 additions & 17 deletions parser/v2/cssparser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package parser

import (
"bytes"
"testing"

"github.com/a-h/parse"
Expand All @@ -9,13 +10,15 @@ import (

func TestExpressionCSSPropertyParser(t *testing.T) {
tests := []struct {
name string
input string
expected ExpressionCSSProperty
name string
input string
expected ExpressionCSSProperty
expectedCSS string
}{
{
name: "css: single constant property",
input: `background-color: { constants.BackgroundColor };`,
name: "css: single constant property",
input: `background-color: { constants.BackgroundColor };`,
expectedCSS: "background-color: ' ';\n",
expected: ExpressionCSSProperty{
Name: "background-color",
Value: StringExpression{
Expand All @@ -38,8 +41,9 @@ func TestExpressionCSSPropertyParser(t *testing.T) {
},
},
{
name: "css: single constant property with windows newlines",
input: "background-color:\r\n{ constants.BackgroundColor };\r\n",
name: "css: single constant property with windows newlines",
input: "background-color:\r\n{ constants.BackgroundColor };\r\n",
expectedCSS: "background-color: ' ';\n",
Copy link
Owner

Choose a reason for hiding this comment

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

For target LSPs to work well, the character positions can't change in the file.

Our goal is to be able to send the current file position (line, col) to the LSP and have it send text insertions and other things back to templ. When we receive those changes, ideally, I'd want to be able to put them into the file without having to remap their positions.

However, this change removes the \r\n and \r chars, therefore changing the line pos.

Here, we're losing \r\n, so the line position

Copy link
Owner

Choose a reason for hiding this comment

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

The expected CSS is presumably the output after parsing, which is essentially the same as what we get if we've formatted the code.

However, in our case, we want to support the user continuously typing into the editor, before they've formatted the code, so I'm not sure this is the right behaviour. Maybe it is, and I'm not understanding the context of the test though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expected CSS is presumably the output after parsing, which is essentially the same as what we get if we've formatted the code.

I believe this to be the behavior that is causing all of these length mismatches. Since the entire document is parsed into a series of nodes and then built back up with the formatting specified in all of the Write() methods, this is the default output after the changes I've made with the ContextWriter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the suggested line length checking to all of the unit tests that I updated. There were quite a few mismatches outside of the ones you caught so those are failing as of 4c7959b.

Copy link
Contributor Author

@alehechka alehechka Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've started on a solution in another branch on my fork: alehechka/templ@feature/html-lsp...alehechka:templ:fix/mismatch-line-lengths.

The rough idea is to include a perservePositions bool on the ContextWriter that will determine how certain nodes get formatted. As of this comment, this is resolving the manipulation of self-closing tags. The next step is reconciling extraneous white space between the source and output.

expected: ExpressionCSSProperty{
Name: "background-color",
Value: StringExpression{
Expand Down Expand Up @@ -76,27 +80,47 @@ func TestExpressionCSSPropertyParser(t *testing.T) {
if diff := cmp.Diff(tt.expected, result); diff != "" {
t.Errorf(diff)
}

w := new(bytes.Buffer)
cw := NewContextWriter(w, WriteContextCSS)
if err := result.Write(cw, 0); err != nil {
t.Fatalf("unexpected error: %v", err)
}
actualCSS := w.String()
if diff := cmp.Diff(tt.expectedCSS, actualCSS); diff != "" {
t.Error(diff)

t.Errorf("input:\n%s", displayWhitespaceChars(tt.input))
t.Errorf("expected:\n%s", displayWhitespaceChars(tt.expectedCSS))
t.Errorf("got:\n%s", displayWhitespaceChars(actualCSS))
}
if diff := cmp.Diff(getLineLengths(tt.input), getLineLengths(actualCSS)); diff != "" {
t.Errorf(diff)
}
})
}
}

func TestConstantCSSPropertyParser(t *testing.T) {
tests := []struct {
name string
input string
expected ConstantCSSProperty
name string
input string
expected ConstantCSSProperty
expectedCSS string
}{
{
name: "css: single constant property",
input: `background-color: #ffffff;`,
name: "css: single constant property",
input: `background-color: #ffffff;`,
expectedCSS: "background-color: #ffffff;\n",
expected: ConstantCSSProperty{
Name: "background-color",
Value: "#ffffff",
},
},
{
name: "css: single constant webkit property",
input: `-webkit-text-stroke-color: #ffffff;`,
name: "css: single constant webkit property",
input: `-webkit-text-stroke-color: #ffffff;`,
expectedCSS: "-webkit-text-stroke-color: #ffffff;\n",
expected: ConstantCSSProperty{
Name: "-webkit-text-stroke-color",
Value: "#ffffff",
Expand All @@ -117,20 +141,40 @@ func TestConstantCSSPropertyParser(t *testing.T) {
if diff := cmp.Diff(tt.expected, result); diff != "" {
t.Errorf(diff)
}

w := new(bytes.Buffer)
cw := NewContextWriter(w, WriteContextCSS)
if err := result.Write(cw, 0); err != nil {
t.Fatalf("unexpected error: %v", err)
}
actualCSS := w.String()
if diff := cmp.Diff(tt.expectedCSS, actualCSS); diff != "" {
t.Error(diff)

t.Errorf("input:\n%s", displayWhitespaceChars(tt.input))
t.Errorf("expected:\n%s", displayWhitespaceChars(tt.expectedCSS))
t.Errorf("got:\n%s", displayWhitespaceChars(actualCSS))
}
if diff := cmp.Diff(getLineLengths(tt.input), getLineLengths(actualCSS)); diff != "" {
t.Errorf(diff)
}
})
}
}

func TestCSSParser(t *testing.T) {
tests := []struct {
name string
input string
expected CSSTemplate
name string
input string
expected CSSTemplate
expectedCSS string
}{
{
name: "css: no parameters, no content",
input: `css Name() {
}`,
expectedCSS: `
`,
expected: CSSTemplate{
Name: "Name",
Expression: Expression{
Expand All @@ -155,6 +199,8 @@ func TestCSSParser(t *testing.T) {
name: "css: without spaces",
input: `css Name() {
}`,
expectedCSS: `
`,
expected: CSSTemplate{
Name: "Name",
Expression: Expression{
Expand All @@ -180,6 +226,9 @@ func TestCSSParser(t *testing.T) {
input: `css Name() {
background-color: #ffffff;
}`,
expectedCSS: `
background-color: #ffffff;
Copy link
Owner

Choose a reason for hiding this comment

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

Similarly, here, we've got a bit of extra spacing.

`,
expected: CSSTemplate{
Name: "Name",
Expression: Expression{
Expand Down Expand Up @@ -210,6 +259,9 @@ background-color: #ffffff;
input: `css Name() {
background-color: { constants.BackgroundColor };
}`,
expectedCSS: `
background-color: ' ';
Copy link
Owner

Choose a reason for hiding this comment

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

And here too.

`,
expected: CSSTemplate{
Name: "Name",
Expression: Expression{
Expand Down Expand Up @@ -256,6 +308,9 @@ background-color: { constants.BackgroundColor };
input: `css Name(prop string) {
background-color: { prop };
}`,
expectedCSS: `
background-color: ' ';
Copy link
Owner

Choose a reason for hiding this comment

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

More additional space.

`,
expected: CSSTemplate{
Name: "Name",
Expression: Expression{
Expand Down Expand Up @@ -312,6 +367,23 @@ background-color: { prop };
if diff := cmp.Diff(tt.expected, result); diff != "" {
t.Errorf(diff)
}

w := new(bytes.Buffer)
cw := NewContextWriter(w, WriteContextCSS)
if err := result.Write(cw, 0); err != nil {
t.Fatalf("unexpected error: %v", err)
}
actualCSS := w.String()
if diff := cmp.Diff(tt.expectedCSS, actualCSS); diff != "" {
t.Error(diff)

t.Errorf("input:\n%s", displayWhitespaceChars(tt.input))
t.Errorf("expected:\n%s", displayWhitespaceChars(tt.expectedCSS))
t.Errorf("got:\n%s", displayWhitespaceChars(actualCSS))
}
if diff := cmp.Diff(getLineLengths(tt.input), getLineLengths(actualCSS)); diff != "" {
t.Errorf(diff)
}
})
}
}
Loading
Loading