-
Notifications
You must be signed in to change notification settings - Fork 29
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: remove variable_definition and assignment rules from pipeline #31
base: master
Are you sure you want to change the base?
Conversation
948b694
to
1e290d6
Compare
I'm not sure about the point about block nodes, it does work, if you access the variable after the block:
We should try to be aligned with the parser of gotemplate and not assume additional logic. |
I only tried to access the variable inside the block and not outside, so I completely missed that. Was my understanding also incorrect in the chained pipeline case? Edit: My example is a valid variable declaration. So a better example would be a variable declaration in the middle of a chained pipeline. |
1e290d6
to
d30ee1b
Compare
Technically this should be allowed:
But it would require more changes in the chained pipeline rule. |
============================================= | ||
Invalid range | ||
============================================= | ||
{{range $var=5}} t1 {{ end }} |
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.
Very interesting,
this is not allowed:
{{ $var:=list }}
{{range $var=list}} t1 {{ end }}
But this is (even executes):
{{range $var:=list}} t1 {{ end }}
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.
Yeah, the second is an element range. I added support for that in #32
test/corpus/pipelines.txt
Outdated
@@ -35,6 +35,21 @@ Function with arguments | |||
(field_identifier)) | |||
(int_literal)))) | |||
|
|||
==================================== | |||
Invalid Function |
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.
Should be called Invalid function parameters.
Also the same thing with with variable definition could be added:
{{ print $var:=5 }}
I think it already is supported: tree-sitter-go-template/test/corpus/examples.txt Lines 135 to 151 in 5f19a36
|
Problem: Rules like `variable_definition`, `assignment`, `range_action`, `template_action`, `chained_pipeline`, `argument_list` incorrectly allowed for variable definitions and assignments inside them. Solution: Remove rules `variable_definition` and `assignment` from `pipeline`, using a new rule where both are supported.
d30ee1b
to
1f4dd25
Compare
Problem
Rules like
variable_definition
,assignment
,range_action
,template_action
,,block_action
chained_pipeline
,argument_list
incorrectly allowed for variable definitions and assignments inside
them.
Solution
Remove rules
variable_definition
andassignment
frompipeline
, using a new rule where both are supported.Go parser doesn't error for variables definitions, assignments inside block andchain operations, despite not working. So I couldn't create a invalid tests
for those cases.
Examples that don't trigger a parsing error:{{block "name" $var1 := "test"}} T1 {{ end }}-> Correction: This is valid{{ $var := 5 | print }}-> This is also a valid variable declaration...We can hack it by mentioning the variable like so:{{block "name" $var := 5}} {{$var}} {{end}}{{ $var := 5 | print $var }}but at this point I don't think it is worth it.