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

[annismckenzie]

GitHub is driving me nuts with their PR collaboration between contributors and maintainers. The same thing that happened in #93 now happened in #95 again. SIGH.

This is the rebased version of #95 onto the current master.

[tooolbox]

Hey @maxime1907 so this is your PR, come to life again!

Just to be clear, this:

• Adds switch with case
• Adds a default pipeline
• Adds filter block with format function
• Does not change the add-operator behavior

Correct? There's been some back-and-forth throughout the comments so I want to make sure I have it straight. (If that's the case, there's not even really breaking changes to existing stuff, just feature adds, which is cool.)

@annismckenzie you said in #95 that you wanted to add more tests:

Sorry that this takes so long but I'm not comfortable with merging this before I add at least a couple more test cases. I will do that this week.

Are you all good now?

Otherwise I guess I can give this branch a spin.

[annismckenzie]

@tooolbox It does change the behavior of the + as can be seen by this test.

[tooolbox]

Got it. Yeah some other discussion on that in #32

So yeah, I see the breaking change...but in thinking about it, how many people intended "1"+2 = "12"?

I will look for this scenario in my tests but now I doubt it's an issue.

[annismckenzie]

No, the default pipeline (I don't really really know what it does OTOH right now… 😅) and the filter block need unit tests.

[tooolbox]

No, the default pipeline (I don't really really know what it does OTOH right now… 😅) and the filter block need unit tests.

@maxime1907 how do you feel about adding unit tests for these things, since you added the features? Otherwise someone may come along and break your desired behavior with a later PR. It seems like you're making Jet (somewhat) compatible with the Perl templating engine, and I don't think any of us have experience with that.

[tooolbox]

...Probably we should land this at some point.

[sauerbraten]

There's a lot in this PR, and I'm not sure if I understand all that's happening, so please someone correct me if I'm wrong:

1. It makes "1"+2 evaluate to 3 instead of "12"
2. It adds {{switch foo}} {{case 1}} 1 {{case 2}} 2 {{case}} something else {{end}}
3. It adds a default built-int function that does nothing but return its only argument (maybe so it can be used in a pipeline?), and also isn't intended to actually be called, and I assume it's meant as a shortcut for foo := isset(foo) && foo ? foo : "some default value" but I honestly don't understand how it's implemented
4. It adds some sort of filter node which apparently pipes its child text node line-by-line through fmt.Sprintf
5. It add a format built-in function that does nothing but return its only argument (?)

I'll go through these points one-by-one:

1. I think it would be good to keep "1"+2 = "12" behavior, which is what JS does and I presume feels more natural to most front-end devs. Is there something else with the + operator that's fixed with this PR? Edit: I just saw it addresses Inconsistency with add operator #32. I'd prefer we go with JS behavior (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Addition#Examples) and make "1"+2 and 1+"2" both evaluate to "12".

2. I'm not a fan of introducing switch and case as nodes, but then use default for something entirely unrelated. Most users will assume the syntax to be switch/case/default like in almost every other language (me included). Regarding the implemetation, I think we can just rewrite the switch/case/default nodes to if/else-if/else and make use of the existing code, similar to how else if x==y is rewritten to else { if x == y {} } already.

3. It seems wrong to be parsing the default/end combo but the to also have default as a function. Maybe there was a reason back then to not just do foo := isset(foo) && foo ? foo : "some default value" (I assume it wasn't done because isset() could panic), but since isset() is now panic-free I gotta say I don't see the benefit of adding a new node type for this.

4. I guess this would be useful to have, but would suggest implementing it differently: allow users to have template text assigned to a variable, then make filter a built-in function. The "template text in a var" thing would have bee useful in Setting variable values from content OR pipelining blocks #167, too, so this just shows we should probably implement that.

5. Reading the diff, I saw some hard-coded checks that look for format as the function name in the expression of a filter node. If we go with a filter as a function, we could let it accept a function (the actual filter implementation) as an argument and have the fmt.Sprintf call in the format function. Sort of like map() in other languages, since filter effectively just maps text to manipulated text.

It would be great if @maxime1907 could chime in, since they authored the original PR, but everyone else's feedback is just as welcome, of course. No matter what we decide on with this PR, I'd also like to split it up into one PR per feature to make it easier to discuss and review.

[tooolbox]

I think it would be good to keep "1"+2 = "12" behavior, which is what JS does and I presume feels more natural to most front-end devs.

No objections.

I'm not a fan of introducing switch and case as nodes, but then use default for something entirely unrelated.

Agreed. @maxime1907 was apparently basing this off of some perl templating which used the keyword as a way to set values to variables if they were falsey. I feel like we could accomplish that a different way.

It seems wrong to be parsing the default/end combo but the to also have default as a function.

Sure.

I guess this would be useful to have, but would suggest implementing it differently: allow users to have template text assigned to a variable, then make filter a built-in function. The "template text in a var" thing would have bee useful in #167, too, so this just shows we should probably implement that.

I am rather keen on being able to say "pipe this block through a formatter", and I'm less fond of storing the result of a block in a variable and then pipelining it.

An example for me is templating an email, where sections of it need to be quoted-printable. The template becomes quite straightforward with just a filter block.

Reading the diff, I saw some hard-coded checks that look for format as the function name in the expression of a filter node. If we go with a filter as a function, we could let it accept a function (the actual filter implementation) as an argument and have the fmt.Sprintf call in the format function. Sort of like map() in other languages, since filter effectively just maps text to manipulated text.

I...think I track?

Instead of this:

{{ filter format("<-- %v -->") }}
This is
formatted text
{{ end }}

output:

<-- This is -->
<-- formatted text -->

Something like this:

{{ filter myFilter }}
This is
formatted text
{{ end }}

output:

// This is
// formatted text

I mean, more powerful, perhaps less obvious from the template what's occurring. But if I need to make stuff quoted-printable, that's hardly visible from the template either.

[sauerbraten]

I'm less fond of storing the result of a block in a variable and then pipelining it.

Can you elaborate on this? I think it would be very useful for people working with long strings, since this way the editor highlighting applies, while keeping the possibility of using functions and pipelines and if/else etc. on those strings, using the existing syntax. (This is pretty much what was requested in #167.)

[tooolbox]

Can you elaborate on this? I think it would be very useful for people working with long strings, since this way the editor highlighting applies, while keeping the possibility of using functions and pipelines and if/else etc. on those strings, using the existing syntax. (This is pretty much what was requested in #167.)

At this point I'm not actually 1000% sure what the tradeoffs are that we're discussing, so I think we should just rewind back slightly. This is what I think would be useful:

{{ filter myFilter }}
This is
formatted text
{{ end }}

output:

// This is
// formatted text

Doing {{filter myFilter | anotherFilter | anotherPipeline}} could also be cool. But the point is you can say "this block gets this formatting" and it's quite straightforward.

What I get from what you're saying is it would be more like this:

{{ block xyz}}
{{ return `
  <html></html>
  `}}
{{ end }}
{{ exec("xyz") | myFiler | anotherFilter }}

It accomplishes the same thing, it just seems more clunky. If {{ return }} was a block statement that applied to everything before {{ end }} (I thought we had that in some way 🤔 ) then it'd be better, but still.

Anyway, there's not a huge difference here I think, if I have the lay of the land right, but that's how I would cast my lot.

[sauerbraten]

Okay, I think I didn't do a good job explaining how I would want filters to work. Let me start with example code:

{{ set foo }}
  some lines
  making up
  a block
{{ end }}
{{ foo | filter: makeComment }}

Assuming makeComment is defined as a custom function like this:

func makeComment(line string) string {
    return "// " + line
}

and registered as a function with Jet, the example Jet code will give you this:

// some line
// making up
// a block

So in essence, (speaking in Go pseudo code), I propose making filter look something like this:

func(f func(string) string, lines ...string) string {
    output := ""
    for _, line := range lines {
        output += f(line) + "\n"
    }
    return output
}

(This omits for simplicity the splitting of the piped value at newlines.)

This way, filters are just functions of a certain signature that you register with Jet, and blocks can be used as value independently of the filter functionality (i.e. would be a better solution for #167).

[tooolbox]

I see what you're saying. I think the only difference is:

Option 1

{{ set foo }}
  some lines
  making up
  a block
{{ end }}
{{ foo | filter: makeComment }}

Option 2

{{ filter makeComment }}
  some lines
  making up
  a block
{{ end }}

I feel like (2) is more...straightforward, less indirection. Less powerful though, I admit.

Option (1) may be easier to implement, since you could reuse all the expression pipelining stuff, so that would be a point in its favor.

Looking at it objectively, it seems like a style preference, and both would be just fine. If you implement (1) I would have nada to complain about and just be happy to have the feature so I can ditch some weird hacks I did 😉

...Was about to post this, and then I thought: do we even need the filter thing? If we have set, (which seems like it would need to be added) you could literally pipe that into a custom function and do the line-breaking yourself. Maybe if we add set we don't need a filter concept in Jet itself, it can be layered on top as required?