Fix inconsistency with add operator, Added switch/case, Added default block values, Filter block with format function #93

maxime1907 · 2018-07-04T10:19:30Z

Add:
"2" + 1 - output: 3
1 + "2" - output: 3
"2" + "2" - output: 4
"2" + "2z" - output: 22z

Switch/Case with empty case as default :
{{ switch power }}
{{ case 10 }}
Value is 10
{{ end }}
{{ case }}
This is default case
{{ end }}
{{ end }}

Default block:

Example 1:
{{ Size = 10 }}
{{ default }}
{{ Size = 20 }}
{{ end }}
Size is {{ Size }} cm
output : Size is 10 cm
Example 2:
{{ default }}
{{ Size = 20 }}
{{ end }}
Size is {{ Size }} cm
output : Size is 20 cm

Filter block with format function:
{{ filter format("<-- %v -->") }}
This is
formatted text
{{ end }}
output:
<-- This is -->
<-- formatted text -->

annismckenzie · 2018-07-04T10:52:04Z

What is happening all of a sudden? Ho-ly! This is major. You seem to have a good grasp on the whole parser and lexer business (*makes-a-mental-note*).

This is a lot to unpack and we'll need multiple rounds of reviews – hope you're up for that. I might also opt for splitting this up into multiple PRs.

I'll do an initial review this evening.

Also, this needs some emojis: 🎉🎉🎉

jhsx · 2018-07-04T18:14:28Z

template.go

@@ -371,6 +372,9 @@ func (scope VarMap) SetWriter(name string, v SafeWriter) VarMap {

 // Execute executes the template in the w Writer
 func (t *Template) Execute(w io.Writer, variables VarMap, data interface{}) error {
+	if w == nil {


This default we should skip, we need to fail when the user pass an nil writer. We can even panic with a descriptive error message.

jhsx · 2018-07-04T18:22:47Z

eval.go

@@ -549,6 +760,38 @@ var (
 	valueBoolFALSE = reflect.ValueOf(false)
 )

+func ParseIndexExpr(baseExpression reflect.Value, indexExpression reflect.Value, indexType reflect.Type) (reflect.Value, error) {


Can you refactor this function in to private ? I think is better to export only the names meant to be used by the user.

I'd also add it to the Runtime struct like the other expression parsing methods.

jhsx · 2018-07-04T18:24:00Z

Thanks for your contributions 👍 .

jhsx · 2018-07-04T18:24:59Z

eval.go

@@ -649,6 +869,40 @@ func (st *Runtime) evalPrimaryExpressionGroup(node Expression) reflect.Value {
 	return st.evalBaseExpressionGroup(node)
 }

+func ParseByType(baseExpression reflect.Value, indexExpression reflect.Value, indexType reflect.Type) (bool, error) {


This one also.

annismckenzie · 2018-07-04T21:16:15Z

I'm gonna pull #32 into the discussion:

I propose a breaking change in the template language, adding a operator for string concatenation and let numeric operators to work only with numbers.
Like php $val.$val2.

I agreed with it then and I agree with it now. Addition should only apply to numeric parts and we should add the dot operator (or something else) to concatenate strings where numeric components are cast to strings. Whatever we decide we should move those code changes to a separate PR that references #32 and if we decide that we instead want it this way then that's fine too. Can you move the commits over to a separate branch, please?

annismckenzie · 2018-07-04T21:22:06Z

examples/todos/main.go

@@ -130,4 +130,4 @@ func main() {

 	log.Println("Serving on " + port)
 	http.ListenAndServe(port, nil)
-}
+}


Please don't drop empty lines at the end of files, it trips up Git's diff algorithm from time to time and makes staging single-line hunks unreliable. If you don't have it yet, add https://editorconfig.org to your editor and configure it with

[*] end_of_line = lf insert_final_newline = true

for the Jet repository (we can add that config file to the .gitignore if need be).

annismckenzie · 2018-07-04T21:22:42Z

lex.go

-	"content": itemContent,
-	"msg":     itemMSG,
-	"trans":   itemTrans,
+	"extends": 	itemExtends,


It doesn't look like you're using go fmt? Always use go fmt in Go projects.

annismckenzie · 2018-07-04T21:22:59Z

node.go

@@ -79,6 +79,10 @@ const (
 	nodeEnd                        //An end action. Not added to tree.
 	NodeField                      //A field or method name.
 	NodeIdentifier                 //An identifier; always a function name.
+	NodeDefault					   //A default action


Same here, go fmt takes care of this for you.

annismckenzie · 2018-07-04T21:24:50Z

.gitignore

+.idea
+sftp-config.json


Undo this change, it doesn't belong.

annismckenzie · 2018-07-04T21:26:41Z

vendor/github.com/CloudyKit/fastprinter/README.md

@@ -1,2 +0,0 @@
-# fastprinter


Please undo removing these files. They were added to prevent people from having to add yet another library dependency and is actually small enough to warrant being in this repository.

annismckenzie · 2018-07-04T21:28:12Z

eval.go

+		switch node.Type() {
+		case NodeAction:
+			node := node.(*ActionNode)
+


drop this empty line

annismckenzie · 2018-07-04T21:29:16Z

eval.go

 					}
 				}
 			}
+		case NodeDefault:
+			node := node.(*DefaultNode)
+


Can you drop the surrounding empty lines here and also in the following case statements?

annismckenzie · 2018-07-04T21:44:11Z

A couple more things and some comments to sum up how I'd like to continue this:

What's the use case {{ filter format("something") }} being added to the language? I can't think of a situation right now where I'd use it. 🤔
The switch/case is valuable, as is the default block so these can go in together. 👍
The add operator inconsistency check code should be moved to a separate branch. That will likely also result in this being included in the v2.2.0 release instead of being moved to v3.0.
Various nits with code formatting.
Test coverage of these changes is 0 which we'll need to change before merging.

In see you committed this straight to your fork's master branch which is a weird thing to do. 😬 So, I can help with all of those chores and nits if you want. You enabled maintainers being able to add commits so just say the word and I'll jump in. It's really great to get contributions and you'll be added to the contributors section in the README which I still haven't done (#60) but will soon. 🎉

mvrhov · 2018-07-05T04:59:08Z

IMO the default shouldn't be a block, but a pipeline "filter".

maxime1907 · 2018-07-05T08:26:58Z

@mvrhov I agree that it would be nice to add the default block to filter functions but the meaning of using a filter is to actually impact what is contained within this node
For instance, we could later add a upper/lower function that will impact inner text, and in a default block you can't do anything other than writing variable assignations
@annismckenzie Thank you for your relevant remarks and to answer your questions:

I need to use this template engine for telephony configuration purposes and a bunch of them are requiring to have a special formatting on all lines
Yes i needed it
Take a look at the issue, i'll answer it
Coding style ^^ but np
I manually test my functions and that's not my cup of tea 😄
You can do whatever you want, i am only interested in adding functionalities for now so change what seems wrong to you :)
Also, take a look at perl template engine http://template-toolkit.org/docs/manual/ , it is very helpful and you will understand why i need a filter block ^^

mvrhov · 2018-07-05T08:34:46Z

No really I don't see the value of a default block.
Why doesn't this work work you? Size is {{ Size|default(20) }} cm, because Size might have not be defined? The default pipe should handle that.
e.g https://twig.symfony.com/doc/2.x/filters/default.html

maxime1907 · 2018-07-05T08:46:44Z

I don't really have a preference, i just looked at http://www.template-toolkit.org/docs/manual/Directives.html#section_DEFAULT and thought it may be a good idea but if you have a preference then let's do this ^^

mvrhov · 2018-07-10T15:35:42Z

👍 for default as a filter implementation. It's more readable and less typing

maxime1907 · 2018-07-12T10:43:10Z

Have i done what you wanted me to fix ? If so, can you consider accepting my pull request ? 😄

annismckenzie · 2018-07-12T13:28:33Z

vendor/github.com/CloudyKit/fastprinter/printers.go

@@ -178,7 +187,7 @@ func PrintValue(w io.Writer, v reflect.Value) (int, error) {
 		return PrintUint(w, v.Uint())
 	}

-	if k == reflect.Float64 || k == reflect.Float32 {
+	if k == reflect.Float64 || k == reflect.Float64 {


Did you copy this in? It's the same comparison on both sides now which is very weird.

no i just cloned https://github.com/CloudyKit/fastprinter/ and https://github.com/CloudyKit/fastprinter/blob/master/printers.go does contain this error ^^

I'll open a separate MR about this later. For now, I reverted to the previous version of the vendored code and opened #94.

annismckenzie · 2018-07-12T13:29:56Z

Thanks for all the changes! I'll be going through this tomorrow. I'm also going to have to do a rebase against the current master.

tooolbox · 2018-07-16T23:08:45Z

@annismckenzie Another use case for "filter" would be to pipe a block through quotedprintable when you're rendering emails to send via smtp.

EDIT: Although...I realize with the way it's implemented, it wouldn't work for that use case 😶

Exciting! Well done @maxime1907

annismckenzie · 2018-07-16T23:29:02Z

Oh GitHub! That »collaborators can just push to that branch« is really really weird and I made a mistake that zeroed out your branch. And because I did that it closed the MR and also removed the permission for me to fix it. I did rework your commits and they still contain your author name and email. Can I trouble you to push your commits again and reopen the MR? So sorry about this. 😖

annismckenzie mentioned this pull request Jul 4, 2018

Inconsistency with add operator #32

Open

jhsx reviewed Jul 4, 2018

View reviewed changes

annismckenzie mentioned this pull request Jul 4, 2018

Add ability to use custom delimiters #91

Merged

2 tasks

annismckenzie reviewed Jul 4, 2018

View reviewed changes

.gitignore Outdated

.idea

sftp-config.json

Copy link

Member

annismckenzie Jul 4, 2018

Choose a reason for hiding this comment

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

Undo this change, it doesn't belong.

annismckenzie reviewed Jul 4, 2018

View reviewed changes

eval.go Outdated

switch node.Type() {

case NodeAction:

node := node.(*ActionNode)

Copy link

Member

annismckenzie Jul 4, 2018

Choose a reason for hiding this comment

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

drop this empty line

annismckenzie reviewed Jul 4, 2018

View reviewed changes

maxime1907 force-pushed the master branch from af0d77d to da6c397 Compare July 9, 2018 15:26

annismckenzie reviewed Jul 12, 2018

View reviewed changes

annismckenzie mentioned this pull request Jul 16, 2018

Update vendored fastprinter library #94

Closed

annismckenzie closed this Jul 16, 2018

annismckenzie force-pushed the master branch from ea526e1 to a14f7be Compare July 16, 2018 23:23

maxime1907 mentioned this pull request Jul 17, 2018

Fix inconsistency with add operator, Added switch/case, Added default pipeline, Filter block with format function #95

Closed

This was referenced Aug 9, 2018

Is there currently a way to escape the default delimiters? #98

Closed

I can custom Delims? #76

Closed

annismckenzie mentioned this pull request Feb 2, 2020

Fix inconsistency with add operator, Added switch/case, Added default pipeline, Filter block with format function (take 3) #136

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inconsistency with add operator, Added switch/case, Added default block values, Filter block with format function #93

Fix inconsistency with add operator, Added switch/case, Added default block values, Filter block with format function #93

maxime1907 commented Jul 4, 2018

annismckenzie commented Jul 4, 2018

jhsx Jul 4, 2018

jhsx Jul 4, 2018

annismckenzie Jul 4, 2018

jhsx commented Jul 4, 2018

jhsx Jul 4, 2018

annismckenzie commented Jul 4, 2018

annismckenzie Jul 4, 2018

annismckenzie Jul 4, 2018

annismckenzie Jul 4, 2018

annismckenzie Jul 4, 2018

annismckenzie Jul 4, 2018

annismckenzie Jul 4, 2018

annismckenzie Jul 4, 2018

annismckenzie commented Jul 4, 2018

mvrhov commented Jul 5, 2018

maxime1907 commented Jul 5, 2018 •

edited

Loading

mvrhov commented Jul 5, 2018 •

edited

Loading

maxime1907 commented Jul 5, 2018 •

edited

Loading

mvrhov commented Jul 10, 2018 •

edited

Loading

maxime1907 commented Jul 12, 2018

annismckenzie Jul 12, 2018

maxime1907 Jul 12, 2018

annismckenzie Jul 16, 2018

annismckenzie commented Jul 12, 2018

tooolbox commented Jul 16, 2018 •

edited

Loading

annismckenzie commented Jul 16, 2018

Fix inconsistency with add operator, Added switch/case, Added default block values, Filter block with format function #93

Fix inconsistency with add operator, Added switch/case, Added default block values, Filter block with format function #93

Conversation

maxime1907 commented Jul 4, 2018

annismckenzie commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhsx commented Jul 4, 2018

Choose a reason for hiding this comment

annismckenzie commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annismckenzie commented Jul 4, 2018

mvrhov commented Jul 5, 2018

maxime1907 commented Jul 5, 2018 • edited Loading

mvrhov commented Jul 5, 2018 • edited Loading

maxime1907 commented Jul 5, 2018 • edited Loading

mvrhov commented Jul 10, 2018 • edited Loading

maxime1907 commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annismckenzie commented Jul 12, 2018

tooolbox commented Jul 16, 2018 • edited Loading

annismckenzie commented Jul 16, 2018

maxime1907 commented Jul 5, 2018 •

edited

Loading

mvrhov commented Jul 5, 2018 •

edited

Loading

maxime1907 commented Jul 5, 2018 •

edited

Loading

mvrhov commented Jul 10, 2018 •

edited

Loading

tooolbox commented Jul 16, 2018 •

edited

Loading