Hi Supun, Great work! I've gone ahead and committed your patch. I especially appreciate your attention to detail in including a good unit test and following our coding style.
I found some minor issues, which I changed before committing. I list them below for future reference. Probably in the future I may bounce some of these back to you for tweaks on your end. (1) I changed the property name to "velocimacro.max.depth" to match similar property "directive.parse.max.depth". (2) Since the patch deals with calling depth, not recursion, I changed all references in comments from "recursion" to "calling depth". (3) In VelocimacroTestCase, there was a subtle reliance on JDK 1.5. (Remember, we require max JDK 1.4 for compiling and 1.3 for running). An int was passed as a parameter when an Integer was expected. Recommendation: set your IDE to enforce JDK 1.4 syntax. (4) In VelocimacroTestCase, one of the tests was repeated. (deleted the extra) (5) You can't change the class Directive, it's one of the public APIs. (yes, this is vague). Basically, any class that can be extended or an interface that can be implemented by changing velocity.properties has to keep the same API. Luckily, since MacroOverflowException is a subclass of RuntimeException you don't need to declare it in the Directive.render() method. WILL On 6/6/07, Will Glass-Husain <[EMAIL PROTECTED]> wrote:
sure, please do! On 6/6/07, Supun Kamburugamuva <[EMAIL PROTECTED]> wrote: > > Hi, > > +1 for velocimacro.depth.max. > > I have alomost completed the implementation. Can I submit a patch? I'm > sure there are lot of things to consider. > > Supun. > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
-- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
