Velocity
  1. Velocity
  2. VELOCITY-666

Blockmacro support (allows any AST as macro body argument)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.2, 1.7
    • Fix Version/s: 1.7
    • Component/s: None
    • Labels:
      None

      Description

      Inspired by VELOCITY-583 (BlockMacro support) I implemented the same functionality in a slightly different way.

      The new syntax is:

      #@yourMacroName($arg1 $arg2) any valid velocity AST here #end

      so basically the syntax is exactly the same as for normal macros except you put that @ prefix to the macro name. That tells Velocity that there's a macro AST body that should be passed to the actual macro.

      And in the macro you can refer to the passed body 0-N times. Like:

      #macro(yourMacroName $foo $bar)
      $bodyContent
      #end

      1. velocity-blockmacro.patch
        22 kB
        Jarkko Viinamäki
      2. velocity-call-directive.patch
        20 kB
        Jarkko Viinamäki

        Issue Links

          Activity

          Hide
          Jarkko Viinamäki added a comment -

          Here's the patch. This issue got ID 666. Nomen ist omen?

          Show
          Jarkko Viinamäki added a comment - Here's the patch. This issue got ID 666. Nomen ist omen?
          Hide
          Claude Brisson added a comment - - edited

          This can be achieved via:

          #set($bodyContent = "#strong($foobar)")

          Can you give us a use case where the new directive would be mandatory?

          PS: you should maybe rename it #summon

          Show
          Claude Brisson added a comment - - edited This can be achieved via: #set($bodyContent = "#strong($foobar)") Can you give us a use case where the new directive would be mandatory? PS: you should maybe rename it #summon
          Hide
          Nathan Bubna added a comment -

          I share your skepticism about committing this. I don't know that

          #macro( strong $txt $bodyContent )
          <strong>$bodyContent</strong> $txt
          #end
          #define( $bodyContent )
          <u>This text is underlined and bold</u>
          #end
          #strong( $foobar $bodyContent )

          Is awkward or long enough to warrant another directive just to shorten it to this

          #macro( strong $txt )
          <strong>$bodyContent</strong> $txt
          #end
          #call( 'strong' $foobar )
          <u>This text is underlined and bold</u>
          #end

          It is hard to find the right balance in these things. Right now, i am inclined to leave this out, as it doesn't seem to provide any needed function, just a somewhat better syntax. And i think what we really want syntactically is this:

          #macro( strong $txt )
          <strong>$bodyContent</strong> $txt
          #end
          #strong( $foobar )
          <u>This text is underlined and bold</u>
          #end

          If that happens, #call will just be a complication for us to deprecate and for users to change uses thereof.

          I also happen to know that there are longtime users who are in the habit of using a #call( $tool.doThis() ) macro and recommending that practice to others. Adding this directive will surely break things for people upgrading.

          I think the bottom line for me is that each directive we add risks conflict with existing macros, increases the famously-low learning curve for Velocity and-as you said-increases the internal complexity/size of Velocity. If we are going to add a new directive, then i think we should be sure that it is well worth the cost. I don't think #call has crossed that threshold, unless i am missing something it can do.

          If it could be implemented in such a way that it didn't require so many internal patches, it would be fine as an optional user directive (like #local). I'm not sure, though, if that is possible for this.

          Show
          Nathan Bubna added a comment - I share your skepticism about committing this. I don't know that #macro( strong $txt $bodyContent ) <strong>$bodyContent</strong> $txt #end #define( $bodyContent ) <u>This text is underlined and bold</u> #end #strong( $foobar $bodyContent ) Is awkward or long enough to warrant another directive just to shorten it to this #macro( strong $txt ) <strong>$bodyContent</strong> $txt #end #call( 'strong' $foobar ) <u>This text is underlined and bold</u> #end It is hard to find the right balance in these things. Right now, i am inclined to leave this out, as it doesn't seem to provide any needed function, just a somewhat better syntax. And i think what we really want syntactically is this: #macro( strong $txt ) <strong>$bodyContent</strong> $txt #end #strong( $foobar ) <u>This text is underlined and bold</u> #end If that happens, #call will just be a complication for us to deprecate and for users to change uses thereof. I also happen to know that there are longtime users who are in the habit of using a #call( $tool.doThis() ) macro and recommending that practice to others. Adding this directive will surely break things for people upgrading. I think the bottom line for me is that each directive we add risks conflict with existing macros, increases the famously-low learning curve for Velocity and- as you said -increases the internal complexity/size of Velocity. If we are going to add a new directive, then i think we should be sure that it is well worth the cost. I don't think #call has crossed that threshold, unless i am missing something it can do. If it could be implemented in such a way that it didn't require so many internal patches, it would be fine as an optional user directive (like #local). I'm not sure, though, if that is possible for this.
          Hide
          Byron Foster added a comment -

          I've thought about the syntax of being able to call a macro like so:

          #macro(bold $txt)<b>$txt</b>#end
          $bold("text")

          This generalizes macro definitions and allows setting:
          #set($bold2 = $bold)
          $bold2("text")

          I'm not real big on #define, it seems like a partial solution to me, mainly because it does not allow parameters, and now you have essentially two types of macros without much gain for the complexity.

          For blocks I think you could simply do:

          #bold("text") #begin <strong>$txt</strong> #end
          or, $bold("text") #begin <strong>$txt</strong> #end

          The macro call would be smart about the #begin block following the macro call and pass it as a parameter. Figures the 666 issues would create a discussion

          Show
          Byron Foster added a comment - I've thought about the syntax of being able to call a macro like so: #macro(bold $txt)<b>$txt</b>#end $bold("text") This generalizes macro definitions and allows setting: #set($bold2 = $bold) $bold2("text") I'm not real big on #define, it seems like a partial solution to me, mainly because it does not allow parameters, and now you have essentially two types of macros without much gain for the complexity. For blocks I think you could simply do: #bold("text") #begin <strong>$txt</strong> #end or, $bold("text") #begin <strong>$txt</strong> #end The macro call would be smart about the #begin block following the macro call and pass it as a parameter. Figures the 666 issues would create a discussion
          Hide
          Nathan Bubna added a comment -

          It wouldn't be hard to allow #define'd blocks to take parameters. Just add a public Block context(Map) method or even a public Block put(key,val) method. Then you could do:

          #define( $intro )$greeting, $audience!#end
          $intro.context(

          {'greeting' : 'Hello', 'audience' : 'World'}

          )

          Really, now that i mention it, i think i may add these when i get a chance; i can already see a place i might use it.

          That said, i'd actually argue that #define adds a great deal of new functionality. You can now assign blocks as references, test for their existence, pass them directly as macro or tool arguments, and we can easily add features like the methods suggested above. I also really appreciate the conceptual separation it gives me. Now, i use #define for inline blocks, and try to limit macros to the global library. This makes it easy for me to know where things come from and where they are going.

          I'm not thrilled about the idea of changing macro syntax to $bold('text'), mostly because we've already trained people the other way, and i don't see much value in being able to assign macros when we can already assign #define blocks. I do, however, think there is potential in the

          #bold("text")#begin<strong>$txt</strong>#end

          idea. Though i think i would prefer the slightly shorter and more instructive #body. I also wonder if it might not be easier for #body to know to look back in the AST for it's macro than for the macro to know to look forward. Anyway, it's an intriguing idea. Obviously, it's still not as ideal as

          #bold('text')<strong>$txt</strong>#end

          but

          #bold('text')#body<strong>$txt</strong>#end

          seems more feasible given the current parser and also seems somehow cleaner and clearer than #call. #body also isn't known to be a popular macro name. I'm still not really sure it adds that much, but the cost feels lower at first glance.

          Show
          Nathan Bubna added a comment - It wouldn't be hard to allow #define'd blocks to take parameters. Just add a public Block context(Map) method or even a public Block put(key,val) method. Then you could do: #define( $intro )$greeting, $audience!#end $intro.context( {'greeting' : 'Hello', 'audience' : 'World'} ) Really, now that i mention it, i think i may add these when i get a chance; i can already see a place i might use it. That said, i'd actually argue that #define adds a great deal of new functionality. You can now assign blocks as references, test for their existence, pass them directly as macro or tool arguments, and we can easily add features like the methods suggested above. I also really appreciate the conceptual separation it gives me. Now, i use #define for inline blocks, and try to limit macros to the global library. This makes it easy for me to know where things come from and where they are going. I'm not thrilled about the idea of changing macro syntax to $bold('text'), mostly because we've already trained people the other way, and i don't see much value in being able to assign macros when we can already assign #define blocks. I do, however, think there is potential in the #bold("text")#begin<strong>$txt</strong>#end idea. Though i think i would prefer the slightly shorter and more instructive #body. I also wonder if it might not be easier for #body to know to look back in the AST for it's macro than for the macro to know to look forward. Anyway, it's an intriguing idea. Obviously, it's still not as ideal as #bold('text')<strong>$txt</strong>#end but #bold('text')#body<strong>$txt</strong>#end seems more feasible given the current parser and also seems somehow cleaner and clearer than #call. #body also isn't known to be a popular macro name. I'm still not really sure it adds that much, but the cost feels lower at first glance.
          Hide
          Jarkko Viinamäki added a comment -

          Hmm I agree that #call... format is a bit ugly. Jonathan is right though that nesting is an important feature that the engine should support.

          Here's another attempt to fix this issue (velocity-blockmacro.patch). I renamed the feature to BlockMacro. I also removed it from directives.properties so it does not clash with anything that already exists.

          The new syntax is:

          #@yourMacroName($arg1 $arg2) any valid velocity AST here #end

          so basically the syntax is exactly the same as for normal macros except you put that @ prefix to the macro name. That tells Velocity that there's a macro AST body that should be passed to the actual macro.

          And in the macro you can refer to the passed body 0-N times. Like:

          #macro(yourMacroName $foo $bar)
          $bodyContent
          #end

          Anyway, since normal Velocity macros are LINE directives, there must be some way to tell Velocity when a body follows the call. I think this is pretty clean syntax.

          Show
          Jarkko Viinamäki added a comment - Hmm I agree that #call... format is a bit ugly. Jonathan is right though that nesting is an important feature that the engine should support. Here's another attempt to fix this issue (velocity-blockmacro.patch). I renamed the feature to BlockMacro. I also removed it from directives.properties so it does not clash with anything that already exists. The new syntax is: #@yourMacroName($arg1 $arg2) any valid velocity AST here #end so basically the syntax is exactly the same as for normal macros except you put that @ prefix to the macro name. That tells Velocity that there's a macro AST body that should be passed to the actual macro. And in the macro you can refer to the passed body 0-N times. Like: #macro(yourMacroName $foo $bar) $bodyContent #end Anyway, since normal Velocity macros are LINE directives, there must be some way to tell Velocity when a body follows the call. I think this is pretty clean syntax.
          Hide
          Nathan Bubna added a comment -

          This looks great, Jarkko. It's a lot cleaner and more B.C. than #call. It's also much closer to the ideal than the #body idea; it's probably as close as we'll get with the current parser. I should be able to take some time after lunch to try this out and check it in. One question, though: is there significance to using @ as the distinguishing character? If not, i wonder if a different character might be more aesthetically pleasing. What do you think about these?

          #:yourMacroName( $arg )
          any ast here
          #end

          #&yourMacroName( $arg )
          any ast here
          #end

          Show
          Nathan Bubna added a comment - This looks great, Jarkko. It's a lot cleaner and more B.C. than #call. It's also much closer to the ideal than the #body idea; it's probably as close as we'll get with the current parser. I should be able to take some time after lunch to try this out and check it in. One question, though: is there significance to using @ as the distinguishing character? If not, i wonder if a different character might be more aesthetically pleasing. What do you think about these? #:yourMacroName( $arg ) any ast here #end #&yourMacroName( $arg ) any ast here #end
          Hide
          Adrian Tarau added a comment - - edited

          I think #@ it's better

          Show
          Adrian Tarau added a comment - - edited I think #@ it's better
          Hide
          Nathan Bubna added a comment -

          Well, assuming Jarkko prefers #@, then i guess i'm outnumbered on thinking #: or #& look better and will leave it as is for now.

          I am, however, going to change BLOCKMACRO_DIRECTIVE_BODYREFERENCE to VM_BODY_REFERENCE and directive.blockmacro.bodyreference to velocimacro.body.reference, since those fit better with the other velocimacro configuration keys.

          Looks great and works well for me. I'll check it in now.

          Show
          Nathan Bubna added a comment - Well, assuming Jarkko prefers #@, then i guess i'm outnumbered on thinking #: or #& look better and will leave it as is for now. I am, however, going to change BLOCKMACRO_DIRECTIVE_BODYREFERENCE to VM_BODY_REFERENCE and directive.blockmacro.bodyreference to velocimacro.body.reference, since those fit better with the other velocimacro configuration keys. Looks great and works well for me. I'll check it in now.
          Hide
          Nathan Bubna added a comment -

          Thanks, Jarkko!

          Show
          Nathan Bubna added a comment - Thanks, Jarkko!
          Hide
          Byron Foster added a comment -

          This is a nice addition, I'm glad you went with #@ syntax, I like it more also.

          Show
          Byron Foster added a comment - This is a nice addition, I'm glad you went with #@ syntax, I like it more also.
          Hide
          Jarkko Viinamäki added a comment -

          Modified the issue title and description to reflect better what was actually implemented.

          Show
          Jarkko Viinamäki added a comment - Modified the issue title and description to reflect better what was actually implemented.
          Hide
          Claude Brisson added a comment -

          Is there any reason not to use #blockmacro instead of introducing a new character? Even Jarkko didn't remember the syntax on the user list... Since 1.7 is not released yet, it's still time to change it.

          Show
          Claude Brisson added a comment - Is there any reason not to use #blockmacro instead of introducing a new character? Even Jarkko didn't remember the syntax on the user list... Since 1.7 is not released yet, it's still time to change it.
          Hide
          Nathan Bubna added a comment -

          Using #blockmacro to define a macro with a body has two major drawbacks for me:

          • it divides macros into two classes, which is not at all an insignificant complication for users, particularly those using macro libraries created by others. #@ allows the macro writer to make macros with the $bodyContent optional and gives template readers/users clear indication of how the macro is being used.
          • more significantly, it introduces implementation headaches when parsing a not-yet-defined macro. See Raghu's 4 options for dealing with this in VELOCITY-583, of which the favored required a new prefix character for using the block macro also.

          It's a new feature. A little forgetfulness can be forgiven.

          Show
          Nathan Bubna added a comment - Using #blockmacro to define a macro with a body has two major drawbacks for me: it divides macros into two classes, which is not at all an insignificant complication for users, particularly those using macro libraries created by others. #@ allows the macro writer to make macros with the $bodyContent optional and gives template readers/users clear indication of how the macro is being used. more significantly, it introduces implementation headaches when parsing a not-yet-defined macro. See Raghu's 4 options for dealing with this in VELOCITY-583 , of which the favored required a new prefix character for using the block macro also. It's a new feature. A little forgetfulness can be forgiven.
          Hide
          Claude Brisson added a comment -

          Ok. Not saying that I'm convinced by the syntax, but looks like I've no alternative (except always sticking to #define to declare the block).

          Show
          Claude Brisson added a comment - Ok. Not saying that I'm convinced by the syntax, but looks like I've no alternative (except always sticking to #define to declare the block).

            People

            • Assignee:
              Unassigned
              Reporter:
              Jarkko Viinamäki
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development