rocker icon indicating copy to clipboard operation
rocker copied to clipboard

support for switch case ?

Open asafbennatan opened this issue 1 year ago • 1 comments

thanks for the great library , coming from handlebars the type safety is a game changer.

i am missing support for switch case ( i registered such function in handlbars)

so i have something like :

 {{#switch name}}
        {{#case 'John'}}
// continue template for John case
        {{/case}}
        {{#case 'Smith'}}
// continue template for Smith case
        {{/case}}
    {{/switch}}

i might be able to add support for this in a PR if this is desirable

asafbennatan avatar Oct 13 '24 08:10 asafbennatan

an update - i have created a fork for my use case , LMK if you want a PR

asafbennatan avatar Oct 13 '24 12:10 asafbennatan

PR created #182

asafbennatan avatar Oct 27 '24 08:10 asafbennatan

New release shipped w/ switch support. Appreciate the contribution.

jjlauer avatar Nov 06 '24 18:11 jjlauer

@jjlauer P.S i believe that the docs are incorrect with regards to switch - there is no @break , it also mentions else if for some reason

asafbennatan avatar Nov 06 '24 22:11 asafbennatan

After merging your PR, I realized you never actually provided a full compiled template example in the template tests module.

Rocker supports java 8 as a minimum and switch expressions are around java 12. So in order to get this to work I added the break statement in my own commits and changed the switch statement to not actually use expressions.

If you have other ideas on modifying this if running on java 12 or higher than it's possible but more complex than what is currently in master.

On Wed, Nov 6, 2024, 5:25 PM asafbennatan @.***> wrote:

@jjlauer https://github.com/jjlauer P.S i believe that the docs are incorrect with regards to switch - there is no @break https://github.com/break , it also mentions else if for some reason

— Reply to this email directly, view it on GitHub https://github.com/fizzed/rocker/issues/181#issuecomment-2460914778, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEPAAVOPJK3OD5KASZWCY3Z7KJM7AVCNFSM6AAAAABP3HO7DSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRQHEYTINZXHA . You are receiving this because you were mentioned.Message ID: @.***>

jjlauer avatar Nov 06 '24 23:11 jjlauer

Also if you look at the commits you can see the compiled template unit test I added which failed on java 8 and 11.

On Wed, Nov 6, 2024, 6:02 PM Joe Lauer @.***> wrote:

After merging your PR, I realized you never actually provided a full compiled template example in the template tests module.

Rocker supports java 8 as a minimum and switch expressions are around java 12. So in order to get this to work I added the break statement in my own commits and changed the switch statement to not actually use expressions.

If you have other ideas on modifying this if running on java 12 or higher than it's possible but more complex than what is currently in master.

On Wed, Nov 6, 2024, 5:25 PM asafbennatan @.***> wrote:

@jjlauer https://github.com/jjlauer P.S i believe that the docs are incorrect with regards to switch - there is no @break https://github.com/break , it also mentions else if for some reason

— Reply to this email directly, view it on GitHub https://github.com/fizzed/rocker/issues/181#issuecomment-2460914778, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEPAAVOPJK3OD5KASZWCY3Z7KJM7AVCNFSM6AAAAABP3HO7DSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRQHEYTINZXHA . You are receiving this because you were mentioned.Message ID: @.***>

jjlauer avatar Nov 06 '24 23:11 jjlauer

ah ok , ill try adding another PR since this change currently breaks for me since i am expecting the switch(..){case(..)->{}..... pattern , specifically i would add that i find it more useful for switching over the type of the template argument which is not possible with java8 switch case

asafbennatan avatar Nov 07 '24 08:11 asafbennatan

actually the issue is that the variation you added includes the @break to support follow through switch case , if this was not added we could have simply generated the break implicitly.

if we want to support both switch case we should probably go with java's syntax:

@switch(something){
case "1": ..... 
case "2" :...... @break //case 1 follow through 
default : .....
}

and for expression based use -> instead of :

@switch(something){
case "1"->{ ..... }
case "2" ->{...... } // no break , followthrough not possible
default -> {}
}

should i create such PR ? @jjlauer

asafbennatan avatar Nov 07 '24 08:11 asafbennatan

I haven't used switch expressions much since most projects these days I'm still stuck on Java 11. Is the @break concept the problem and you'd like an implicit break OR is the expression -> { } itself what you're looking for?

I thought about not requiring @break and making it implicit -- would that solve you're main issue? If that's your main issue, we can still change things around by not requiring an @break and maybe do it implicitly or signal passthrough a different way.

If you really need switch expressions -> { } that's absolutely possible, but a bit more complicated. You'd need to only target and output them on Java 14? and above (rocker already detects java versions so that part isn't hard). Maven is the other annoying problem, we'd need to make a rocker-test-template17 module that targets Java 17+, then in the parent pom.xml, include that module only if the JVM is 17 and above. Thus, if running on Java 8-16, that module is skipped for testing/compiling during unit tests. I previously had examples of this when I had Java 6/7 compat, which I just removed in Rocker 1.5.0. You can see how this worked by expanding the entire pom.xml from this commit: https://github.com/fizzed/rocker/commit/590399d1b70df0b59d5b92bc449e4441fc2662d0#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8

jjlauer avatar Nov 07 '24 13:11 jjlauer

the problem is that i didnt have @break statements in my templates so ill have to add this to make this work , but i think if you want to the normal switch case we should do it as i mentioned above and keep the feature the way i developed it only for JDK 14+

doing it this way would allow us support both type switches adhering to java syntax, if we keep it as it is now it will be a problem since you wont be able to differentiate between switch expression and normal switch with follow through

asafbennatan avatar Nov 07 '24 13:11 asafbennatan

Switch expressions do look cool and maybe there are other Java 17+ features we would eventually add, so I'd welcome the addition of support if you'd like to add it. As I mentioned in my previous comment, the key part is making a new maven module called rocker-test-template17, which mirrors how rocker-test-template works, but its source/target is set to Java 17, and then is only conditionally included as a module in the main parent pom.xml if running on java 17+.

If you run into problems supporting the -> operator in the Antlr parsing, we could also potentially look at making it called something like @switch_e or something instead and keeping the rest of how its structured the same. But I do like the idea of including -> in the case statements to differentiate that you're using switch expressions vs. standard switches.

jjlauer avatar Nov 07 '24 13:11 jjlauer

this required changing the switch syntax we have right now to be something like : @switch(..){ case:....@break case:...@break } is this fine?

asafbennatan avatar Nov 07 '24 14:11 asafbennatan

Not sure what you mean by the syntax, can you do a full example of switching on something like a String?

jjlauer avatar Nov 07 '24 14:11 jjlauer

yes ,

currently in version 2.0.1 we do :

@args (String s)
@switch (s) {
   case("test"){ this is a test @break}
   case("test2"){ this is a test2 @break}
}

which generates :

switch(s){
   case "test": 
       handlePlainText("this is a test");// dummy
       break;
   case "test2": 
       handlePlainText("this is a test2");
       break;               
}

in my branch version we had :

@args (String s)
@switch (s) {
   case("test"){ this is a test }
   case("test2"){ this is a test2 }
}

which generates :

switch(s){
   case "test"->{ 
       handlePlainText("this is a test");// dummy
       }
   case "test2"->{
       handlePlainText("this is a test2");
       }    
}


what i am saying is that we cannot simply use the same syntax for rocker but make it without the break statement since it will be ambiguous if its follow through or expression.

do you think we should support an alternative rocket syntax for switch expression such as :

@args (String s)
@switch (s) {
   case("test")->{ this is a test }
   case("test2")->{ this is a test2 }
}

which generates :

switch(s){
   case "test"->{ 
       handlePlainText("this is a test");// dummy
       }
   case "test2"->{
       handlePlainText("this is a test2");
       }    
}

asafbennatan avatar Nov 07 '24 14:11 asafbennatan

Yes, I'd be supportive of your example "switch expression" alternative syntax if that's what you'd really like. I explained in my previous comments what all that would entail since its only valid for Java 14+, which we could simplify to be more like 17+ (to pick an LTS version to test against)

jjlauer avatar Nov 07 '24 15:11 jjlauer

Ok , already underway , expect a pr soon

On Thu, Nov 7, 2024, 17:58 Joe Lauer @.***> wrote:

Yes, I'd be supportive of your example "switch expression" alternative syntax if that's what you'd really like. I explained in my previous comments what all that would entail since its only valid for Java 14+, which we could simplify to be more like 17+ (to pick an LTS version to test against)

— Reply to this email directly, view it on GitHub https://github.com/fizzed/rocker/issues/181#issuecomment-2462603627, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACET7TDQ5MX4AFI3I6OO6UDZ7OE3FAVCNFSM6AAAAABP3HO7DSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRSGYYDGNRSG4 . You are receiving this because you authored the thread.Message ID: @.***>

asafbennatan avatar Nov 07 '24 16:11 asafbennatan

@jjlauer see #186

asafbennatan avatar Nov 07 '24 20:11 asafbennatan