Render final keyword for @for loops in java source or source may not compile about rocker HOT 10 CLOSED

Three comments.

1. Are you using Java 7 or below? If Java 8+ you can skip adding a type on the for statement or the with statement.

2. We unit test for Java 7 w/ for & with statements, do you do any sort of custom javac flag like enforcing warnings or such? Curious if we missed something in our own unit tests that could help prevent a regression down the road.

3. Yes, absolutely would welcome a PR w/ open arms. Guessing its as easy as tweaking https://github.com/fizzed/rocker/blob/master/rocker-compiler/src/main/java/com/fizzed/rocker/compiler/JavaGenerator.java to add the final keyword in various places.

from rocker.

Thanks for your reply.

1. Java 8, but the rocker java options set to 6 (it's needs to be source compatible for java 6).
2. Not that I know of, the compiler was just the real java compiler failing to compile a generated java template class. I will see if I can reproduce it with a separate simple testcase.
3. Ok, I hope I can reproduce 2 and with that fix the compile error.

I'll get back to you soon.

from rocker.

Ok, I reproduced it.

I had a look through the code and tests, and I will add a relevant test file to rocker-test-java6/src/test/java/rocker

Since I expect it to fail for java 8 as well, shall I add them to rocker-test-java8/src/test/java/rocker too? If so, I will modify them to use the java 8 syntactic sugar.

from rocker.

If the fixes for both are different then yes you'd want to add tests to rocker-test-java6 and rocker-test-java8. Looking fwd to the fix! If you can get the PR over to me in the next 4 hours I'll be happy to push it to maven central.

from rocker.

Thanks, that would be awesome! I'll get the fixes in and create the pull request.

There is one specific loop (normal one) I cannot fix this quickly right now as I need to have a better look at the code for that, but you will see that in the test file (I will add the failing test, but disable that specific one). I don't mind looking into that one later, but that will have to wait for the weekend as I have more time then. If that's ok with you, I'll have a look on that one then.

The current fixes should cover all for loop related issues except that one, as far I can see now.
I will add the same tests for the java 8 ones just in case, as they do not hurt (that file also contains the disabled failing test).

from rocker.

Ok, pull request created. Hope it's ok. :)

See my comment above, so you can decide how to deal with the current disabled failing test.

from rocker.

Hey Joe,

I had a look at the remaining issue. This one is a lot harder to tackle, and we should consider not even trying to fix it (as it's actually how normal java works here). Since we're dealing with ordinary loops like:

@for(int i = 0; i < x; i++)

For one thing 'i' can never be final, since it's part of a loop after all and does change. In theory you could workaround this, by doing the loop in a different fashion and generating it differently, but then you need to fully parse the expression first before you can do that (atm this is stored in ForStatement as 3 parts). Don't think it's really worth the effort.

What I think would make more sense (and that's how you would deal with it in normal java anyway) is to have a separate final assignent variable.

E.g. like the @with but in a non-scoped way.

Samples of what I was thinking about:

@assign (String newObject =	key.toLowerCase())
@assign (int index = i)

These assigns will be made final in generation then and are available in the template.

This would be a very useful addition, I'd love to have them: specifically to assign things used repeatedly in a template, as well as for expensive methods calls, I rather execute only once than many times.

The above will also work for the for loop:

@for (int i = 0; i < x; i++) {
  @assign (int index=i)
  @with (...) {
    @index // Ok
  }
}

What are your thoughts?

from rocker.

A couple thoughts. I agree its too hard to support the standard syntax. Parsing the inner expression is a lot of work for this one case. Not sure i'm sold on an @assign syntax. I initially designed the @with syntax to act more like your proposal and I ran into some strange scenarios w/o scoping the assignment like @with currently does. Perhaps an easier solution is to support Java 8 streams in for loops or some type of adapter object which allows one to use the enhanced syntax as an alternative.

from rocker.

I see, what were the issues you used to have with the scoping-less @with?
I guess it's in the end the responsibility for the template writer, not to abuse a variable name. I'd personally see it as a final variable really, which cannot be assigned again. I agree the @with is nicer normally, still the @assign might be useful in some instances.

That aside, now that we're talking about the @with. What would be nice as well, to have the with accept multiple arguments for assignment. So you can define multiple scoped arguments. Which would make the code really clean, without having to nest multiple times if you need several assignments.

I don't have the code here when writing this comment, so I do not know if that would be doable or incredibly hard to support that, as it is now. If it's doable, I may be willing to put time in that. What do you think?

Could you put the current version as is - in maven btw, then I can use an official build at work again. :)

from rocker.

@mreuvers v0.14.0 is released. Will take a couple hours to hit maven central I'd guess.

I think setting multiple arguments in a @with statement would be very useful and a much welcomed feature if you wanted to submit a PR. Going to close this issue. Thx for your contribution!

from rocker.