Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.7, 2.x
    • Component/s: Engine
    • Labels:
      None

      Description

      In the discussion for VELOCITY-680, Claude suggested the addition of what i'm calling "control" objects as a solution. These would have the same name as the block directive or macro to which they belong. At a minimum, these would provide get(key), set(key, value) and stop() methods to control the reference scoping and execution of the block to which they belong. Directives could extend the basic control object to provide additional functions, such as index() and hasNext() for #foreach. Here's some examples:

      #foreach( $user in $users )
      $user#if( $foreach.hasNext ), #end
      #if( $foreach.count > 10 ) $foreach.stop() #end
      #end

      #macro( foo $bar )
      blah blah #if( $bar == 'bar' ) $foo.stop() #end
      #set( $foo.woogie = 'woogie' )
      $foo.woogie
      #end

      #foreach( $item in $list )
      #set( $outer = $foreach )
      #foreach( $attr in $item.attributes )
      #if ( $attr == $null ) $outer.stop()#end
      #end
      #end

      -----foo.vm--------
      blah blah $template.stop() blah
      ------------------------

      #define( $foo )
      blah blah $define.stop() blah
      #end

      This could allow us to greatly simplify all sorts of things. We could remove the #break, #stop and #return directives. We would no longer need to have "local" contexts for foreach loops or macros; instead users could set and get local variables directly in the provided namespace. All else would be global. This may even cut down our internal code complexity a fair bit. It'll certainly obviate the need for several configuration properties and internal contexts. Everything becomes much more explicit, obvious and robust. I also don't think it looks ugly.

      We would, of course, have to make sure that the StopExceptions thrown by stop() aren't wrapped into MethodInvocationExceptions. We'd have to make the directives clean up their control when done rendering, and if they're nested in a directive of the same type, then they should save and restore the reference to the parent control. We'd also have to figure out a good default name to give the control objects for the top-level control object, and whether it would be different than the name of the control object used during a #parse call. $template? $parse? $velocity? If we wanted to use $template-which i think works well for both top-level and #parse-then we'd probably have to make it configurable, since that's likely to conflict. And if we make that configurable, i suppose we may as well make it configurable for the others too.

      I'm struggling to think of any real downside to this. Most of the replaced features (implicit macro localscope, #stop, #break, $velocityHasNext) are either not default behavior or are new features. I'd wager that most people would only have to change $velocityCount to $foreach.count. Even that's no big deal, since this would be for a major version change. , The worst i can think of is the fact that for a couple of these controls it would mean a few more keystrokes. Considering all the gains in extensibility, explicitness and simplification (for us and users), i think it's worth a few keystrokes.

      What do you guys think?

        Activity

        Hide
        Nathan Bubna added a comment -

        Ok, now all are off by default, except $foreach. I also did some quick tests and try-catch was faster than instanceof, so i swapped those too. I am still thinking it would be better to keep a stack of ScopeOwners in the InternalContextAdapter and create them on demand (which would also obviate the whole Scope.getReplaced() thing), but i'm not sure i'll get to that soon. First, i'm gonna copy these changes in the 2.x branch.

        Show
        Nathan Bubna added a comment - Ok, now all are off by default, except $foreach. I also did some quick tests and try-catch was faster than instanceof, so i swapped those too. I am still thinking it would be better to keep a stack of ScopeOwners in the InternalContextAdapter and create them on demand (which would also obviate the whole Scope.getReplaced() thing), but i'm not sure i'll get to that soon. First, i'm gonna copy these changes in the 2.x branch.
        Hide
        Nathan Bubna added a comment -

        I was thinking about this and realized that we really shouldn't have any of these turned on by default, except for $foreach. I believe this is the only one that provides things otherwise available with the default settings now. The others would be extras and only really needed when someone wants a local scope and/or wants to #break to a particular $<scope>, neither of which is likely to be standard use.

        Having $macro, $template, $evaluate, $define and $<macroname> off by default should reduce the overhead to a boolean check. Those who wish to use $macro or the like, may turn them on, just as they currently have to turn on the now-deprecated velocimacro.context.localscope.

        Show
        Nathan Bubna added a comment - I was thinking about this and realized that we really shouldn't have any of these turned on by default, except for $foreach. I believe this is the only one that provides things otherwise available with the default settings now. The others would be extras and only really needed when someone wants a local scope and/or wants to #break to a particular $<scope>, neither of which is likely to be standard use. Having $macro, $template, $evaluate, $define and $<macroname> off by default should reduce the overhead to a boolean check. Those who wish to use $macro or the like, may turn them on, just as they currently have to turn on the now-deprecated velocimacro.context.localscope.
        Hide
        Nathan Bubna added a comment -

        Ay, a speed hit is expected. Though i think it is worth it for the features and simplification it offers. I do hope we can improve on the speed. Of course, if you don't use the feature and want faster macros, the simplest fix is:

        macro.provide.scope.control = false

        then it's just a boolean condition check in both preRender and postRender.

        Also, in 2.0, some of the lost ground should be recovered (i expect) since 1.7 has a lot of now-deprecated functionality that has been removed from the 2.0 branch (velocimacro.context.localscope, velocityCount, velocityHasNext, etc

        As for particular speed improvements, i haven't done any testing or much thinking yet, but when i first wrote this i did consider replacing a lot of those:

        if (obj instanceof Scope)

        { this } else { that }

        with

        try { this }

        catch (ClassCastException)

        { that }

        since i think it will be rather rare (and increasingly so in the future) that people will override Scope instances in their context. But, without doing any performance testing to compare the two approaches, i figured it was best to start out doing it "the right way" and worry about optimizing later. The one other idea i had was to keep a small map of "scoped directive" stacks (or something like that) in the InternalContextAdapter and lazily create Scope objects as requested. It seemed more difficult to do at the time though, and again, without performance numbers to compare it was hard to justify the effort. Anyway, all that is to say that i think we can do better here performance-wise; it'll just take a little testing and refactoring.

        Show
        Nathan Bubna added a comment - Ay, a speed hit is expected. Though i think it is worth it for the features and simplification it offers. I do hope we can improve on the speed. Of course, if you don't use the feature and want faster macros, the simplest fix is: macro.provide.scope.control = false then it's just a boolean condition check in both preRender and postRender. Also, in 2.0, some of the lost ground should be recovered (i expect) since 1.7 has a lot of now-deprecated functionality that has been removed from the 2.0 branch (velocimacro.context.localscope, velocityCount, velocityHasNext, etc As for particular speed improvements, i haven't done any testing or much thinking yet, but when i first wrote this i did consider replacing a lot of those: if (obj instanceof Scope) { this } else { that } with try { this } catch (ClassCastException) { that } since i think it will be rather rare (and increasingly so in the future) that people will override Scope instances in their context. But, without doing any performance testing to compare the two approaches, i figured it was best to start out doing it "the right way" and worry about optimizing later. The one other idea i had was to keep a small map of "scoped directive" stacks (or something like that) in the InternalContextAdapter and lazily create Scope objects as requested. It seemed more difficult to do at the time though, and again, without performance numbers to compare it was hard to justify the effort. Anyway, all that is to say that i think we can do better here performance-wise; it'll just take a little testing and refactoring.
        Hide
        Jarkko Viinamäki added a comment -

        Duh. Based on my velocity testbench (with test case similar to one in VELOCITY-24) the 1.7-dev (current head) is about 20% slower than 1.6.2.

        Profiling shows that in 1.7-dev most of the time is spent on RuntimeMacro.render and VelocimacroProxy.render. This is due to new preRender and postRender methods and new scoping functionality. Since there is some instanceof-checking, object creation and hashmap overhead, it will be difficult to improve this. It also affects all macro calls regardless if you use the new functionality or not. :/

        Show
        Jarkko Viinamäki added a comment - Duh. Based on my velocity testbench (with test case similar to one in VELOCITY-24 ) the 1.7-dev (current head) is about 20% slower than 1.6.2. Profiling shows that in 1.7-dev most of the time is spent on RuntimeMacro.render and VelocimacroProxy.render. This is due to new preRender and postRender methods and new scoping functionality. Since there is some instanceof-checking, object creation and hashmap overhead, it will be difficult to improve this. It also affects all macro calls regardless if you use the new functionality or not. :/
        Hide
        Nathan Bubna added a comment -

        That should do it, i think.

        Show
        Nathan Bubna added a comment - That should do it, i think.
        Hide
        Nathan Bubna added a comment -

        Ok, not quite done with code changes... I just realized i overlooked one pocket of implicit, non-optional local context: EvaluateContext. Will, are you tracking this? If so, do you recall the reasons for making context changes within #evaluate local? This is rather different than VelocityTools' ViewRenderTool which uses the current context.

        Also, there's support for using custom context classes for that local scope. I'm trying to think of the use-case for this but drawing a complete blank. I obviously need to deprecate it in 1.7 and yank it from 2.0 to be consistent, but i'm still curious about what it was for. Also, i'm tempted to revert to global scope for 1.7 unless the user specifies a local context class. Thoughts?

        Show
        Nathan Bubna added a comment - Ok, not quite done with code changes... I just realized i overlooked one pocket of implicit, non-optional local context: EvaluateContext. Will, are you tracking this? If so, do you recall the reasons for making context changes within #evaluate local? This is rather different than VelocityTools' ViewRenderTool which uses the current context. Also, there's support for using custom context classes for that local scope. I'm trying to think of the use-case for this but drawing a complete blank. I obviously need to deprecate it in 1.7 and yank it from 2.0 to be consistent, but i'm still curious about what it was for. Also, i'm tempted to revert to global scope for 1.7 unless the user specifies a local context class. Thoughts?
        Hide
        Nathan Bubna added a comment -

        Ok, i think that should do it code-wise. Now for documentation...

        Show
        Nathan Bubna added a comment - Ok, i think that should do it code-wise. Now for documentation...
        Hide
        Nathan Bubna added a comment -

        Ok, i've ported the changes to 2.0 and started cleaning out the things it replaces. So far, #return, $velocityCount and $velocityHasNext are gone. Next on the chopping block is velocimacro.context.localscope, then #global, #local and inner supporting code. You'll probably notice that it is a bit awkward to remove one part at a time. Since this is something of a paradigm shift, some of the related tests don't even make sense now. Bear with me if some of the intermediate stages have odd loose ends. I do think though, that doing it in stages should be easier to follow than taking them all out at once.

        Once all is cleaned up, i'll start work on the documentation updates as i have time.

        Show
        Nathan Bubna added a comment - Ok, i've ported the changes to 2.0 and started cleaning out the things it replaces. So far, #return, $velocityCount and $velocityHasNext are gone. Next on the chopping block is velocimacro.context.localscope, then #global, #local and inner supporting code. You'll probably notice that it is a bit awkward to remove one part at a time. Since this is something of a paradigm shift, some of the related tests don't even make sense now. Bear with me if some of the intermediate stages have odd loose ends. I do think though, that doing it in stages should be easier to follow than taking them all out at once. Once all is cleaned up, i'll start work on the documentation updates as i have time.
        Hide
        Nathan Bubna added a comment -

        The current implementation has it printed to the logs as a debug string "StopCommand: <message here>". I can't imagine doing anything else with it but debugging.

        Show
        Nathan Bubna added a comment - The current implementation has it printed to the logs as a debug string "StopCommand: <message here>". I can't imagine doing anything else with it but debugging.
        Hide
        Byron Foster added a comment -

        I like your proposal Nathan, and I like Claude's suggested alteration.

        I'm curious about the #stop string parameter. Where would this be displayed? It seems that an #error or #assert directive would be a better fit for indicating errors, or debugging. but I'm indifferent. Hmm, I like #assert though...

        Show
        Byron Foster added a comment - I like your proposal Nathan, and I like Claude's suggested alteration. I'm curious about the #stop string parameter. Where would this be displayed? It seems that an #error or #assert directive would be a better fit for indicating errors, or debugging. but I'm indifferent. Hmm, I like #assert though...
        Hide
        Claude Brisson added a comment -

        except maybe that we can keep #stop($<scope>) (so #<scope>(scope/string) is overloaded), #break being always invoked without args

        Show
        Claude Brisson added a comment - except maybe that we can keep #stop($<scope>) (so #<scope>(scope/string) is overloaded), #break being always invoked without args
        Hide
        Claude Brisson added a comment -

        Fine for me.

        Show
        Claude Brisson added a comment - Fine for me.
        Hide
        Nathan Bubna added a comment -

        Another vote to keep #break around, eh? How about this:

        #break -> stop innermost scope
        #break($<scope>) -> stop specified scope
        #stop -> stop outermost scope w/no message
        #stop("info string") -> stop outermost scope w/message

        I can't say i consider this to be as simple or straightforward. But since we do already have both directives in 1.x and #break seems to have some fans, i can live with this. I'll just rationalize that all of the users who would bother with these features won't be confused by having two such similar directives. After all, we already have the #define and #macro cousins, and i've yet to figure out a clean way to consolidate those two either.

        Show
        Nathan Bubna added a comment - Another vote to keep #break around, eh? How about this: #break -> stop innermost scope #break($<scope>) -> stop specified scope #stop -> stop outermost scope w/no message #stop("info string") -> stop outermost scope w/message I can't say i consider this to be as simple or straightforward. But since we do already have both directives in 1.x and #break seems to have some fans, i can live with this. I'll just rationalize that all of the users who would bother with these features won't be confused by having two such similar directives. After all, we already have the #define and #macro cousins, and i've yet to figure out a clean way to consolidate those two either.
        Hide
        Claude Brisson added a comment -

        What about :
        #break -> cancel most inner scope
        #stop -> exit merging (and rather than the scope, I'd give it an info string, since it's often the way to handle error cases)
        this looks rather backward compatible and intuitive

        anyway, this is a side debate since this syntax is accessible with macros (I'd let the Scope.stop() anyway).

        Show
        Claude Brisson added a comment - What about : #break -> cancel most inner scope #stop -> exit merging (and rather than the scope, I'd give it an info string, since it's often the way to handle error cases) this looks rather backward compatible and intuitive anyway, this is a side debate since this syntax is accessible with macros (I'd let the Scope.stop() anyway).
        Hide
        Claude Brisson added a comment -

        I'm pleased with your choises so far - what should the behaviour of #stop be when invoked without any argument?

        • most global scope = current behaviour
        • most local scope = much more interesting imho
        Show
        Claude Brisson added a comment - I'm pleased with your choises so far - what should the behaviour of #stop be when invoked without any argument? most global scope = current behaviour most local scope = much more interesting imho
        Hide
        Byron Foster added a comment -

        You know I think these discussions get buried in these tickets. For example in this ticket there are really about 4 issues being discussed.

        About the #evaluate scope, I really don't have an opinion, I have never used this directive so I don't have a feel for what makes since in practice. If you have a strong persuasion, and there is no other opinion, then go with your gut

        I'm indifferent about #pragma, but I don't think it solves the default scoping behavior since those of us who prefer the default to be macro would want it system wide, and don't want to have to specify the behavior on every page. This takes us then to a velocity.properties setting, and ...

        Ok, I don't like to be the last word type, but I do have to respectively address your presentation of defaulting to global scope Your labels of "implicit scoping" vs "explicit scoping" I think are not accurate. Look, the only question is what does #set($foo = "bar") do??? I assert that the default behavior should be macro scope (No side effects), you assert global scope. I don't think either approach is more "explicit" or "implicit".

        I agree with everything else you say. I like this approach in general to scoping. And yes, it's a huge improvement over where VTL is now where I don't think even the committers could give a concise set of rules that describe current scoping behavior. Adding scope control, and formalizing the behavior is good for everybody.

        Yes, I hated $foreach.stop() #stop($foreach) is much better, even though I have a strong feeling I'll continue to use #break, the second class directive, I'm a creature of habit!

        Show
        Byron Foster added a comment - You know I think these discussions get buried in these tickets. For example in this ticket there are really about 4 issues being discussed. About the #evaluate scope, I really don't have an opinion, I have never used this directive so I don't have a feel for what makes since in practice. If you have a strong persuasion, and there is no other opinion, then go with your gut I'm indifferent about #pragma, but I don't think it solves the default scoping behavior since those of us who prefer the default to be macro would want it system wide, and don't want to have to specify the behavior on every page. This takes us then to a velocity.properties setting, and ... Ok, I don't like to be the last word type, but I do have to respectively address your presentation of defaulting to global scope Your labels of "implicit scoping" vs "explicit scoping" I think are not accurate. Look, the only question is what does #set($foo = "bar") do??? I assert that the default behavior should be macro scope (No side effects), you assert global scope. I don't think either approach is more "explicit" or "implicit". I agree with everything else you say. I like this approach in general to scoping. And yes, it's a huge improvement over where VTL is now where I don't think even the committers could give a concise set of rules that describe current scoping behavior. Adding scope control, and formalizing the behavior is good for everybody. Yes, I hated $foreach.stop() #stop($foreach) is much better, even though I have a strong feeling I'll continue to use #break, the second class directive, I'm a creature of habit!
        Hide
        Nathan Bubna added a comment -

        Alright, we'll stick with $evaluate as the default for RuntimeInstance.evaluate(...) and #evaluate. It's easy enough for users to do #if(!$template)#set( $template = $evaluate)#end at the top if they really aren't sure (for some strange reason) whether the VTL they're writing will be run through a resource loader or through evaluate and do want to use the scoped variable storage. As i write that, i realize all the more how unlikely that is...

        Christoph, am i right that this #pragma you suggest would generally be a way to change Velocity configuration from within a template? Probably best a discussion for a separate thread, if so. Interesting idea, not sure what i think of it yet.

        Well, i think i'll start working on this more (porting to 2.x and documenting), as you're making me more hopeful of some level of consensus. Doesn't seem like anyone else has strong opinions here but us; even Claude, who birthed this idea seems quiet now.

        I know what i'm pushing for here isn't likely to be an easy pill to swallow for those used to using velocimacro.context.localscope and other such "implicit scoping" in languages, but i am sincerely convinced that this "explicit scoping" is an overall win that will keep VTL more readable, reusable and robust. It allows users much more control and flexibility than before at a bare minimum of new concepts/code/complication. I also think it is going to simplify development and user support efforts. So, despite the unfamiliarity of this approach, if you bear with me and try to focus on what it adds and the potential it could have, i think you may come to appreciate it! That said, if you do see any more problems with it or flaws in my thinking, please keep pointing them out. I'm already grateful for your pushback on this, as without it, i would likely have stopped with the stop() method and not moved to the much better #stop that is now implemented (not that you're sold on it yet, i know .

        Show
        Nathan Bubna added a comment - Alright, we'll stick with $evaluate as the default for RuntimeInstance.evaluate(...) and #evaluate. It's easy enough for users to do #if(!$template)#set( $template = $evaluate)#end at the top if they really aren't sure (for some strange reason) whether the VTL they're writing will be run through a resource loader or through evaluate and do want to use the scoped variable storage. As i write that, i realize all the more how unlikely that is... Christoph, am i right that this #pragma you suggest would generally be a way to change Velocity configuration from within a template? Probably best a discussion for a separate thread, if so. Interesting idea, not sure what i think of it yet. Well, i think i'll start working on this more (porting to 2.x and documenting), as you're making me more hopeful of some level of consensus. Doesn't seem like anyone else has strong opinions here but us; even Claude, who birthed this idea seems quiet now. I know what i'm pushing for here isn't likely to be an easy pill to swallow for those used to using velocimacro.context.localscope and other such "implicit scoping" in languages, but i am sincerely convinced that this "explicit scoping" is an overall win that will keep VTL more readable, reusable and robust. It allows users much more control and flexibility than before at a bare minimum of new concepts/code/complication. I also think it is going to simplify development and user support efforts. So, despite the unfamiliarity of this approach, if you bear with me and try to focus on what it adds and the potential it could have, i think you may come to appreciate it! That said, if you do see any more problems with it or flaws in my thinking, please keep pointing them out. I'm already grateful for your pushback on this, as without it, i would likely have stopped with the stop() method and not moved to the much better #stop that is now implemented (not that you're sold on it yet, i know .
        Hide
        Christoph Reck added a comment -

        At the template level a new directive #pragma would be handy - with such a capability, the programmer could be able to override the default scope:
        #pragma( scope "template" )
        or
        #pragma( scope "evaluate" )

        To keep the simplicity of the directives, it also could be:
        #set( $pragma.scope = "template" )

        OT: this idea has been in my mind since the whitespace handling threads:
        #pragma( whitespacehandling "structured" )

        my 2c.

        Show
        Christoph Reck added a comment - At the template level a new directive #pragma would be handy - with such a capability, the programmer could be able to override the default scope: #pragma( scope "template" ) or #pragma( scope "evaluate" ) To keep the simplicity of the directives, it also could be: #set( $pragma.scope = "template" ) OT: this idea has been in my mind since the whitespace handling threads: #pragma( whitespacehandling "structured" ) my 2c.
        Hide
        Byron Foster added a comment -

        I think having an independent $evaluate over $template is appropriate. Given the usage of #evaluate it seems that the developer would want the option to specify locally scoped variables.

        As to your performance questions the numbers are due to throughput, so total time to execute the benchmark. I didn't dive into it anymore then that. LIke you say, it's a little early to worry too much about performance numbers, even though I do tend to be an efficiency hound

        I think I understand more where you're coming from. I think I'v had a slight bend toward a more programmatic approach and syntax, with strong macro support. My usage is more web work where heavy macro usage and structured templating is needed, at least for highly dynamic functionality.

        Maybe I'm too engaged in my mindset, I'm going to take a step back and try to see the forest

        Show
        Byron Foster added a comment - I think having an independent $evaluate over $template is appropriate. Given the usage of #evaluate it seems that the developer would want the option to specify locally scoped variables. As to your performance questions the numbers are due to throughput, so total time to execute the benchmark. I didn't dive into it anymore then that. LIke you say, it's a little early to worry too much about performance numbers, even though I do tend to be an efficiency hound I think I understand more where you're coming from. I think I'v had a slight bend toward a more programmatic approach and syntax, with strong macro support. My usage is more web work where heavy macro usage and structured templating is needed, at least for highly dynamic functionality. Maybe I'm too engaged in my mindset, I'm going to take a step back and try to see the forest
        Hide
        Nathan Bubna added a comment -

        Yes, #set( $template.foo = 'bar' ) works for most cases, unless you are running via engine.evaluate(), in which case $evaluate would be the global scope. Which brings up a nagging thought. Should the scope for evaluate() also be $template? I've tried to name the scope according to the context in which the VTL is being written. The name for all of them was thus pretty obvious, except for engine.evaluate(...) and #evaluate(...). Yes, they're different from #parse or Template.merge, since you can directly put a string in when calling them, but that string is always a template. Thoughts?

        10-20% performance hit on what? Memory? Speed? And where is the hit? Directive.init()? #stop? #set? I'd love to know more, though admittedly i'm more concerned with getting features settled before optimizing anything.

        As for #stop vs #break and #return, can you explain to me why you don't see that? I feel like i've made my case already, but here's one more try: i think 1 being "simpler" than 3 is pretty obvious--less to document, less to debug, less to know. I think it is clearly "better" because advanced users have complete control and are not limited to merely stopping the most immediate foreach or macro. And "intuition" is terribly subjective. I'll only argue that it is more VTL-centric because it doesn't introduce new terms from other languages and the baggage they carry (break labels and return values). I was really hoping to find some consensus between you and i about this, but if we can't get that, then i'd love to hear from anyone else following this discussion. What do you think?

        No need to speculate about my macro usage, i've said before that prior to 1.6's fixes and improvements, i considered it reckless to rely on them heavily. Still, i have been using Velocity over 8 years now, so i might have made up some in longevity of what i lacked in breadth? Who knows. And yeah, i share the "traditional" opposition to thinking of macros as a "method call" construct (thus my opposition to associating them with terms like 'return'). Changing #set to default to localscope for 2.0 is a non-BC change that i currently would consider inconsistent (what about #parse and #define?), unnecessary (as a user, i've never seen fit to turn velocimacro.context.localscope to true, and as a dev, it's been an occasional nuisance) and generally lacking in compelling reasons to exist (all the moreso given the introduction of $macro). Still, if you think you have a good case for it, i would love to hear you out. Though perhaps in a different thread?

        Show
        Nathan Bubna added a comment - Yes, #set( $template.foo = 'bar' ) works for most cases, unless you are running via engine.evaluate(), in which case $evaluate would be the global scope. Which brings up a nagging thought. Should the scope for evaluate() also be $template? I've tried to name the scope according to the context in which the VTL is being written. The name for all of them was thus pretty obvious, except for engine.evaluate(...) and #evaluate(...). Yes, they're different from #parse or Template.merge, since you can directly put a string in when calling them, but that string is always a template. Thoughts? 10-20% performance hit on what? Memory? Speed? And where is the hit? Directive.init()? #stop? #set? I'd love to know more, though admittedly i'm more concerned with getting features settled before optimizing anything. As for #stop vs #break and #return, can you explain to me why you don't see that? I feel like i've made my case already, but here's one more try: i think 1 being "simpler" than 3 is pretty obvious--less to document, less to debug, less to know. I think it is clearly "better" because advanced users have complete control and are not limited to merely stopping the most immediate foreach or macro. And "intuition" is terribly subjective. I'll only argue that it is more VTL-centric because it doesn't introduce new terms from other languages and the baggage they carry (break labels and return values). I was really hoping to find some consensus between you and i about this, but if we can't get that, then i'd love to hear from anyone else following this discussion. What do you think? No need to speculate about my macro usage, i've said before that prior to 1.6's fixes and improvements, i considered it reckless to rely on them heavily. Still, i have been using Velocity over 8 years now, so i might have made up some in longevity of what i lacked in breadth? Who knows. And yeah, i share the "traditional" opposition to thinking of macros as a "method call" construct (thus my opposition to associating them with terms like 'return'). Changing #set to default to localscope for 2.0 is a non-BC change that i currently would consider inconsistent (what about #parse and #define?), unnecessary (as a user, i've never seen fit to turn velocimacro.context.localscope to true, and as a dev, it's been an occasional nuisance) and generally lacking in compelling reasons to exist (all the moreso given the introduction of $macro). Still, if you think you have a good case for it, i would love to hear you out. Though perhaps in a different thread?
        Hide
        Byron Foster added a comment -

        On the pro side I think the approach of #set($template.foo = "bar") works, which accommodates the original problem being solved from VELOCITY-680.

        The experimental benchmark shows about a 10-20% performance hit, but this may not be a big deal in practice, and this may be improved.

        I'v tried hard, but I just don't see #stop($foreach) or #stop($macro) to be simpler, more intuitive or better then #break and #return respectively. I'm sure I"m biased by my programming experience.

        I think defaulting #set to global scope within a macro block is the wrong way to go, but you know that . I speculate that you don't use macros very much, and that maybe this is the more natural approach for such usage. However, maybe this fits into the tradition Velocity notion of macros as "saving keystrokes", and less of a programming construct.

        Anyway, my 3 cents.

        Show
        Byron Foster added a comment - On the pro side I think the approach of #set($template.foo = "bar") works, which accommodates the original problem being solved from VELOCITY-680 . The experimental benchmark shows about a 10-20% performance hit, but this may not be a big deal in practice, and this may be improved. I'v tried hard, but I just don't see #stop($foreach) or #stop($macro) to be simpler, more intuitive or better then #break and #return respectively. I'm sure I"m biased by my programming experience. I think defaulting #set to global scope within a macro block is the wrong way to go, but you know that . I speculate that you don't use macros very much, and that maybe this is the more natural approach for such usage. However, maybe this fits into the tradition Velocity notion of macros as "saving keystrokes", and less of a programming construct. Anyway, my 3 cents.
        Hide
        Nathan Bubna added a comment -

        Ok, #stop is now a directive that takes an optional Scope argument and #break uses StopCommand.

        I've also been thinking more about $global, and i think it's unnecessary. In the 1.x trunk, there is already no means for setting a global variable if velocimacro.context.localscope is turned on. So, there's no backward compatibility issue in not having it. And it wouldn't make sense to add it, since i'll be removing velocimacro.context.localscope in 2.0. We just need to actively deprecate velocimacro.context.localscope and encourage/warn those who use it to switch to using the $macro namespace when they want to keep a macro var locally scoped.

        In regard to deprecating the replaced features, here's my thoughts for 1.x:

        • Deprecate velocimacro.context.localscope by warning users who set it to true in the logs that they should use $macro instead.
        • Deprecate #break and put a notice in the logs that users should use #stop($foreach) instead.
        • When users have non-default velocityCount or velocityHasNext settings, warn them in the logs that they should switch to $foreach.count and $foreach.hasNext
        • Update all the documentation to refer only to the new stuff.
        • Leave these things as such for the remainder of 1.x's lifetime. Only actually remove them in 2.0.
        Show
        Nathan Bubna added a comment - Ok, #stop is now a directive that takes an optional Scope argument and #break uses StopCommand. I've also been thinking more about $global, and i think it's unnecessary. In the 1.x trunk, there is already no means for setting a global variable if velocimacro.context.localscope is turned on. So, there's no backward compatibility issue in not having it. And it wouldn't make sense to add it, since i'll be removing velocimacro.context.localscope in 2.0. We just need to actively deprecate velocimacro.context.localscope and encourage/warn those who use it to switch to using the $macro namespace when they want to keep a macro var locally scoped. In regard to deprecating the replaced features, here's my thoughts for 1.x: Deprecate velocimacro.context.localscope by warning users who set it to true in the logs that they should use $macro instead. Deprecate #break and put a notice in the logs that users should use #stop($foreach) instead. When users have non-default velocityCount or velocityHasNext settings, warn them in the logs that they should switch to $foreach.count and $foreach.hasNext Update all the documentation to refer only to the new stuff. Leave these things as such for the remainder of 1.x's lifetime. Only actually remove them in 2.0.
        Hide
        Nathan Bubna added a comment -

        You're right, i left that off the todo list unintentionally, thanks for the reminder. I didn't do it yet partly because $global will only be useful in 1.x, while velocimacro.context.localscope exists and is turned on. Once that is gone, the global scope will be what #set( $foo = 'bar' ) always sets $foo into, no configuration or starting point based ambiguity. But more to the point, i was undecided about how best to do it. Right now, i'm leaning toward always putting the context within itself under the key "global". Then the $global scope won't have any of the other Scope features, but at least will be a way to set global vars from within a macro's localscope.

        Next, though, i'm working on the revamp of #stop and making #break use $foreach.stop(). $global will follow those changes.

        Show
        Nathan Bubna added a comment - You're right, i left that off the todo list unintentionally, thanks for the reminder. I didn't do it yet partly because $global will only be useful in 1.x, while velocimacro.context.localscope exists and is turned on. Once that is gone, the global scope will be what #set( $foo = 'bar' ) always sets $foo into, no configuration or starting point based ambiguity. But more to the point, i was undecided about how best to do it. Right now, i'm leaning toward always putting the context within itself under the key "global". Then the $global scope won't have any of the other Scope features, but at least will be a way to set global vars from within a macro's localscope. Next, though, i'm working on the revamp of #stop and making #break use $foreach.stop(). $global will follow those changes.
        Hide
        Byron Foster added a comment -

        I'll try it out. What about $global ?

        Show
        Byron Foster added a comment - I'll try it out. What about $global ?
        Hide
        Nathan Bubna added a comment -

        Ok, i checked my first pass at this into the trunk. Here's the basics:

        Scopes are automatically added under the following keys for the listed situations:

        $template in Template.merge and #parse content
        $evaluate in RuntimeInstance.evaluate and #evaluate content
        $foreach in #foreach bodies
        $macro in #macro definitions
        $define in #define definitions
        $<macroname> in #@<macroname> body content

        All of the above can:
        -serve as a map for storing scoped variable.
        -access any parent scope of the same kind when nested via $<name>.parent
        -access the topmost parent scope via $<name>.topmost
        -access any reference they overrode via $<name>.replaced
        -tell you what their nesting depth is via $<name>.depth
        -stop rendering via $<name>.stop(), though this feature will be removed once you can do #stop($<name>)

        $foreach can also give you the 0-based $foreach.index, 1-based $foreach.count, $foreach.hasNext, $foreach.first and $foreach.last

        These things still need to be (in my estimation):

        • a revamp of #stop to take a Scope as an argument
        • refactoring the throwables used by ASTStop and Break to be proper kin of StopCommand
        • decision about whether/how we should deprecate $velocityCount, $velocityHasNext, #break, velocimacro.context.localscope, etc now or later?
        • abuse and critical review from you guys
        • documentation, including changelog notice
        • merge/port to 2.x branch

        Right now, i mostly want to hear your impressions...

        Show
        Nathan Bubna added a comment - Ok, i checked my first pass at this into the trunk. Here's the basics: Scopes are automatically added under the following keys for the listed situations: $template in Template.merge and #parse content $evaluate in RuntimeInstance.evaluate and #evaluate content $foreach in #foreach bodies $macro in #macro definitions $define in #define definitions $<macroname> in #@<macroname> body content All of the above can: -serve as a map for storing scoped variable. -access any parent scope of the same kind when nested via $<name>.parent -access the topmost parent scope via $<name>.topmost -access any reference they overrode via $<name>.replaced -tell you what their nesting depth is via $<name>.depth -stop rendering via $<name>.stop(), though this feature will be removed once you can do #stop($<name>) $foreach can also give you the 0-based $foreach.index, 1-based $foreach.count, $foreach.hasNext, $foreach.first and $foreach.last These things still need to be (in my estimation): a revamp of #stop to take a Scope as an argument refactoring the throwables used by ASTStop and Break to be proper kin of StopCommand decision about whether/how we should deprecate $velocityCount, $velocityHasNext, #break, velocimacro.context.localscope, etc now or later? abuse and critical review from you guys documentation, including changelog notice merge/port to 2.x branch Right now, i mostly want to hear your impressions...
        Hide
        Nathan Bubna added a comment -

        The more i think about it, the more i think that having one #stop that optionally takes and uses a control object is a better solution all around. It provides the benefits mentioned above and also means we don't have to deprecate and remove #stop, which would be good since it is the oldest of the three directives in question.

        Show
        Nathan Bubna added a comment - The more i think about it, the more i think that having one #stop that optionally takes and uses a control object is a better solution all around. It provides the benefits mentioned above and also means we don't have to deprecate and remove #stop, which would be good since it is the oldest of the three directives in question.
        Hide
        Nathan Bubna added a comment -

        I'm not sure i understand why $foreach.stop() needs any more validation than any other reference. #return (and #break if modified as you describe) only needs the extra parse-time validation because they throw an exception if used in improper contexts. Admittedly, i may be blind to the drawback since i don't use or develop a smart Velocity editor.

        Still, if it's just a directive solution you want and you're willing to have just one instead of three, then perhaps we should combine the concepts and have the new #stop take a control object as an optional argument:

        #stop - stops all merge activity
        #stop($foreach) - stops just the current #foreach
        #stop($foreach.parent) - stops all the way up to the parent #foreach

        Then it doesn't look like we're emitting content, is totally VTL-centric, has all the flexibility and explicitness of stop(), and can even be validated at parse time. The parse-time validation is possible because the naming of control objects is only dependent on velocity.properties, not runtime contextual info.

        If this were to be the direction we took, then it can also easily take advantage of my imminent commit for this case. We'd just move the stop() method to #stop or leave it in the Control class and make it protected so only #stop can call it, not templates.

        Would that be a better solution for you?

        Show
        Nathan Bubna added a comment - I'm not sure i understand why $foreach.stop() needs any more validation than any other reference. #return (and #break if modified as you describe) only needs the extra parse-time validation because they throw an exception if used in improper contexts. Admittedly, i may be blind to the drawback since i don't use or develop a smart Velocity editor. Still, if it's just a directive solution you want and you're willing to have just one instead of three, then perhaps we should combine the concepts and have the new #stop take a control object as an optional argument: #stop - stops all merge activity #stop($foreach) - stops just the current #foreach #stop($foreach.parent) - stops all the way up to the parent #foreach Then it doesn't look like we're emitting content, is totally VTL-centric, has all the flexibility and explicitness of stop(), and can even be validated at parse time. The parse-time validation is possible because the naming of control objects is only dependent on velocity.properties, not runtime contextual info. If this were to be the direction we took, then it can also easily take advantage of my imminent commit for this case. We'd just move the stop() method to #stop or leave it in the Control class and make it protected so only #stop can call it, not templates. Would that be a better solution for you?
        Hide
        Byron Foster added a comment -

        <qoute>
        this is for a 2.0 version? I'm positive. I use the "varStatus" attribute of JSTL's <c:foreach> all the time. Seems similar but with the addition of methods.
        </qoute>

        The better example of what is being discussed here is:

        c:forEach items="$

        {list}

        " var="item">
        <c:if test="$

        {forEach.first}

        "> $

        {forEach.break}

        </c:if>
        Item # $

        {forEach.count}

        is $

        {item}

        .
        </c:forEach>

        Show
        Byron Foster added a comment - <qoute> this is for a 2.0 version? I'm positive. I use the "varStatus" attribute of JSTL's <c:foreach> all the time. Seems similar but with the addition of methods. </qoute> The better example of what is being discussed here is: c:forEach items="$ {list} " var="item"> <c:if test="$ {forEach.first} "> $ {forEach.break} </c:if> Item # $ {forEach.count} is $ {item} . </c:forEach>
        Hide
        Byron Foster added a comment -

        <quote>For instance, #break exists all the time, even when not in a #foreach</qoute>

        That's only because it is implemented that way. If you look at the #return directive it validates that it's in a #macro block at template load time (during init). #break could do the same. However, we can do better, my plan was to beef up the directive validation at parse time, then Velocity smart IDE's like Veloeclipse could validate #break and #return as the user edits her VTL. This level of validation is simply not an option for .stop().

        <qoute>It is obvious what $foreach.stop() does</qoute>

        This is all very subjective of course, that's why there's a hundred million languages out there. However, people with a programming background, most users I would think, are going to instantly understand what #break does (and #return for that matter). And to the non-programmers it's not going to make any difference. To me, again subjective, $foreach.stop() looks like a reference that's emitting content. I'm not against $foreach.stop() constructs, I'm more indifferent, I'm just arguing for a directive solution also. However, I can tell you're against a dual solution, and yes I think you could make a good argument for this, so you don't need to

        I regret having proposed #stop(parse) or #break(glass). I think it confused the issue, when all that was of real interest to me was #return and adding an 'index' keyword to #foreach. I was thinking #return would get as much resistance as this VELOCITY-612

        <qoute>then i would go back to pushing hard for just one #stop() directive</qoute>

        I'll take it. So, something like this:

        #stop(all)
        #stop(parse)
        #stop(macro) = #return
        #stop(foreach) = #break

        What would #stop do?

        Show
        Byron Foster added a comment - <quote>For instance, #break exists all the time, even when not in a #foreach</qoute> That's only because it is implemented that way. If you look at the #return directive it validates that it's in a #macro block at template load time (during init). #break could do the same. However, we can do better, my plan was to beef up the directive validation at parse time, then Velocity smart IDE's like Veloeclipse could validate #break and #return as the user edits her VTL. This level of validation is simply not an option for .stop(). <qoute>It is obvious what $foreach.stop() does</qoute> This is all very subjective of course, that's why there's a hundred million languages out there. However, people with a programming background, most users I would think, are going to instantly understand what #break does (and #return for that matter). And to the non-programmers it's not going to make any difference. To me, again subjective, $foreach.stop() looks like a reference that's emitting content. I'm not against $foreach.stop() constructs, I'm more indifferent, I'm just arguing for a directive solution also. However, I can tell you're against a dual solution, and yes I think you could make a good argument for this, so you don't need to I regret having proposed #stop(parse) or #break(glass). I think it confused the issue, when all that was of real interest to me was #return and adding an 'index' keyword to #foreach. I was thinking #return would get as much resistance as this VELOCITY-612 <qoute>then i would go back to pushing hard for just one #stop() directive</qoute> I'll take it. So, something like this: #stop(all) #stop(parse) #stop(macro) = #return #stop(foreach) = #break What would #stop do?
        Hide
        Nathan Bubna added a comment -

        Yes, you can think of it as a varStatus with the addition of a few methods for local variable storage and for stopping the current directive's rendering.

        And i actually intend to introduce it in 1.7 to start the transition to this (and thus away from $velocityCount, $velocityHasNext, #break, #stop and velocimacro.context.localscope) long before 2.0 is released. This way, we don't have to carry around unneeded, deprecated code for 2.0.

        Show
        Nathan Bubna added a comment - Yes, you can think of it as a varStatus with the addition of a few methods for local variable storage and for stopping the current directive's rendering. And i actually intend to introduce it in 1.7 to start the transition to this (and thus away from $velocityCount, $velocityHasNext, #break, #stop and velocimacro.context.localscope) long before 2.0 is released. This way, we don't have to carry around unneeded, deprecated code for 2.0.
        Hide
        Will Glass-Husain added a comment -

        this is for a 2.0 version? I'm positive. I use the "varStatus" attribute of JSTL's <c:foreach> all the time. Seems similar but with the addition of methods.

        <c:forEach items="$

        {list}

        " var="item" varStatus="status">

        <c:if test="$

        {status.first}

        "> First </c:if>

        Item # $

        {status.count}

        is $

        {item}

        .

        </c:forEach>

        Show
        Will Glass-Husain added a comment - this is for a 2.0 version? I'm positive. I use the "varStatus" attribute of JSTL's <c:foreach> all the time. Seems similar but with the addition of methods. <c:forEach items="$ {list} " var="item" varStatus="status"> <c:if test="$ {status.first} "> First </c:if> Item # $ {status.count} is $ {item} . </c:forEach>
        Hide
        Nathan Bubna added a comment -

        The stop() function won't have any performance penalty over the directive versions. There will be a small cost for creating the map to hold the scoped variables, but in a few directives (localscope macros, #evaluate and #foreach), we already create inner contexts. I really don't think we'll see any cost to the scope.

        I don't think stop() is any more confusing than having three differently named "stop" directives that have no explicit connection to the situations for which they are meant to be used. For instance, #break exists all the time, even when not in a #foreach. It is obvious what $foreach.stop() does, and it can't be used elsewhere since it will only exist within a #foreach. To easily confused users, it may not be at all obvious why you use #break to end a #foreach early, then use #stop(parse) to end a #parse early, and then can't leave a macro early unless you add an undocumented directive with yet another name. The current system is cobbled together from updates of old debugging code and half-implemented features from other languages. In stark contrast, the stop() approach is consistent throughout and is very VTL-centric, which means less for users to think about.

        Even if it does confuse people at first, it shouldn't confuse many of them. #stop only became usable in v1.5 and has never been very documented, #break only showed up a couple months ago in v1.6 and #return and #stop(parse) are unreleased. These are not widely used features, and those most likely to use them are more advanced users who are unlikely to be confused. And for such advanced users, the greater power and explicitness of stop() should be a great boon. For instance, when you have nested #foreach, #parse or #macro situations, then you cannot get out of more than one at once. With the control reference, they will be able to do $foreach.stopAll() or even be specific and do things like $foreach.parent.stop().

        Even if i were somehow persuaded to avoid having a method affect directive execution (which LoopTool already does), then i would go back to pushing hard for just one #stop() directive. I see no reason to keep three of them around and very good reasons to have just one.

        stop() provides one consistent, explicit, flexible way to do this for all situations. It is also simple to implement and therefore easy to debug and maintain for us developers as well.

        Show
        Nathan Bubna added a comment - The stop() function won't have any performance penalty over the directive versions. There will be a small cost for creating the map to hold the scoped variables, but in a few directives (localscope macros, #evaluate and #foreach), we already create inner contexts. I really don't think we'll see any cost to the scope. I don't think stop() is any more confusing than having three differently named "stop" directives that have no explicit connection to the situations for which they are meant to be used. For instance, #break exists all the time, even when not in a #foreach. It is obvious what $foreach.stop() does, and it can't be used elsewhere since it will only exist within a #foreach. To easily confused users, it may not be at all obvious why you use #break to end a #foreach early, then use #stop(parse) to end a #parse early, and then can't leave a macro early unless you add an undocumented directive with yet another name. The current system is cobbled together from updates of old debugging code and half-implemented features from other languages. In stark contrast, the stop() approach is consistent throughout and is very VTL-centric, which means less for users to think about. Even if it does confuse people at first, it shouldn't confuse many of them. #stop only became usable in v1.5 and has never been very documented, #break only showed up a couple months ago in v1.6 and #return and #stop(parse) are unreleased. These are not widely used features, and those most likely to use them are more advanced users who are unlikely to be confused. And for such advanced users, the greater power and explicitness of stop() should be a great boon. For instance, when you have nested #foreach, #parse or #macro situations, then you cannot get out of more than one at once. With the control reference, they will be able to do $foreach.stopAll() or even be specific and do things like $foreach.parent.stop(). Even if i were somehow persuaded to avoid having a method affect directive execution (which LoopTool already does), then i would go back to pushing hard for just one #stop() directive. I see no reason to keep three of them around and very good reasons to have just one. stop() provides one consistent, explicit, flexible way to do this for all situations. It is also simple to implement and therefore easy to debug and maintain for us developers as well.
        Hide
        Byron Foster added a comment -

        This is more then what I was thinking of what a "control" object would be, I was thinking it was more of a Map of variables type thing for managing scope.

        I'm worried that it might be confusing that VTL directive execution is being modified by reference looking objects like $macro.stop(). However, I'm indifferent to it as long as there isn't a performance hit.

        I prefer #stop, #break, and #return. #return is already relegated to the closet since it seems no Velocity committers like it . I would prefer to keep #break and #stop around. But, if committers are down on these also then I'd like to put them in the closet also.

        Show
        Byron Foster added a comment - This is more then what I was thinking of what a "control" object would be, I was thinking it was more of a Map of variables type thing for managing scope. I'm worried that it might be confusing that VTL directive execution is being modified by reference looking objects like $macro.stop(). However, I'm indifferent to it as long as there isn't a performance hit. I prefer #stop, #break, and #return. #return is already relegated to the closet since it seems no Velocity committers like it . I would prefer to keep #break and #stop around. But, if committers are down on these also then I'd like to put them in the closet also.
        Hide
        Nathan Bubna added a comment -

        We could give nested controls access to their parent of the same type like this:

        #foreach( $item in $list )
        #foreach( $attr in $item.attributes)
        #if( $attr == $null ) $foreach.parent.stop() #end
        $

        {foreach.parent.count}

        .$

        {foreach.count}

        : $attr
        #end
        #end

        No need to do the #set( $outer = $foreach ).

        I'm also thinking that we may be able to start introducing these control objects in 1.x. This could allow us to start deprecating the replaced features (#stop, #break, $velocityCount, $velocityHasNext, macro localscope setting) prior to 2.0. Given my frustrating and continuing experience with VelocityTools 2.0, i would like 2.0 to be as clean of deprecations as possible, and not try hard to maintain compatibility for 1.x. Better to start the transition in 1.x wherever reasonable.

        Show
        Nathan Bubna added a comment - We could give nested controls access to their parent of the same type like this: #foreach( $item in $list ) #foreach( $attr in $item.attributes) #if( $attr == $null ) $foreach.parent.stop() #end $ {foreach.parent.count} .$ {foreach.count} : $attr #end #end No need to do the #set( $outer = $foreach ). I'm also thinking that we may be able to start introducing these control objects in 1.x. This could allow us to start deprecating the replaced features (#stop, #break, $velocityCount, $velocityHasNext, macro localscope setting) prior to 2.0. Given my frustrating and continuing experience with VelocityTools 2.0, i would like 2.0 to be as clean of deprecations as possible, and not try hard to maintain compatibility for 1.x. Better to start the transition in 1.x wherever reasonable.

          People

          • Assignee:
            Unassigned
            Reporter:
            Nathan Bubna
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development