On Thu, Jan 14, 2016 at 6:51 PM, vincent(a)massol.net wrote: +1, it's a pain to call setLastError() everywhere there can be an exception thrown, and we almost always forget to do it (for this reason). Note that we also have the #try() directive now.

Thanks, Marius > > Thanks > -Vincent > > PS: This is http://jira.xwiki.org/browse/XWIKI-2374

On Fri, Jan 15, 2016 at 9:18 AM, vincent(a)massol.net wrote: Does it mean you plan to get rid of new #try directive ? Because it will be broken with this new uberspector.

> Thanks > -Vincent > >> Thanks, >> Marius >> >> >> > >> > Thanks >> > -Vincent >> > >> > PS: This is http://jira.xwiki.org/browse/XWIKI-2374

On 15 Jan 2016 at 10:38:05, vincent(a)massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote: Actually, thinking more, I think you’re right and that the #try directive plays exactly the same role as an Exception-catching uberspector and I don’t see the need for the #try directive if we provide an uberspector. So I’m proposing to deprecate it but still keep it for backward compatibility for now (probably a full cycle). WDYT?

Thanks -Vincent > Thanks > -Vincent > > > > Thanks > > > -Vincent > > > > > >> Thanks, > > >> Marius > > >> > > >> > > >> > > > >> > Thanks > > >> > -Vincent > > >> > > > >> > PS: This is http://jira.xwiki.org/browse/XWIKI-2374

hmm… this would mean that I’d need to put this new uberspector in xwiki-platform since VelocityManager is in platform ATM… (@Thomas: our discussion of yesterday ;)).

Forget what I’ve said regarding the #try directive. Actually the way to use it is: #try() … wrap some script snipper, one or several calls… #end .. here only you can access the $exception binding. Thus: * Wrapping the whole content to evaluate won’t work * The Exception Catching Uberspector is really what we need. It’s more fine-grained than the #try directive (or it’s equivalent if you use #try() for each line of script calling methods). In addition: * We provide script APIs in xwiki-commons too so if we want this best practice of throwing exceptions in script APIs, we also need to make this work in xwiki-commons when used outside of XWiki. Thus we need a solution there too. ** A) One solution would be to use XWiki’s Context and make the last exception available with $xcontext.lastVelocityException but it’s not very natural, I hope we can find a better solution ** B) Another solution is to move VelocityManager to commons in xwiki-commons-velocity, offer a basic impl without skin support and override it in xwiki-platform to add the platform-specific stuff. I’ll start exploring B). Let me know if you don’t agree or if you have a better idea. 

Thanks -Vincent On 15 Jan 2016 at 12:15:41, vincent(a)massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote: > > [snip] > > > > > > > >> > Hi devs, > > > > > > >> > > > > > > > >> > Right now our strategy is for script services and script APIs in general > > > > > > >> > to catch exceptions, store them and offer a getLastError() method to get > > > > > > >> > them (see > > > > > > >> > http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra… > > > > > > >> > ) > > > > > > >> > > > > > > > >> > However it would be much nicer to: > > > > > > >> > * Let our script services generate exceptions > > > > > > >> > * Offer a velocity script service to get the last exception raised by a > > > > > > >> > java call from velocity > > > > > > >> > * Implement this uberspector to catch the exceptions and to set them in > > > > > > >> > the execution context > > > > > > >> > > > > > > > >> > That should be quite easy to implement IMO. > > > > > > >> > > > > > > > >> > WDYT? > > > > > > >> > > > > > > > >> > > > > > > >> +1, it's a pain to call setLastError() everywhere there can be an exception > > > > > > >> thrown, and we almost always forget to do it (for this reason). > > > > > > >> > > > > > > >> Note that we also have the #try() directive now. > > > > > > > > > > > > > > Yes, I should have mentioned that there’s indeed also this possibility: > > > > > > > * Have script API throw Exceptions > > > > > > > * Force velocity script users to wrap their code with the try directive when they need to catch exceptions > > > > > > > > > > > > > > I still believe that the use of the Exception-catching uberspector is better. > > > > > > > > > > > > > > WDYT? > > > > > > > > > > > > Does it mean you plan to get rid of new #try directive ? Because it > > > > > > will be broken with this new uberspector. > > > > > > > > > > That’s a good point, I had not thought about the implementation at this stage. > > > > > > > > > > I think this could still work. When the #try directive is used I’d just have to setup some flag somewhere in Velocity and in the uberspector I could check if this flag is set and if so then don’t catch the exception. > > > > > > > > > > > > Actually, thinking more, I think you’re right and that the #try directive plays exactly the same role as an Exception-catching uberspector and I don’t see the need for the #try directive if we provide an uberspector. > > > > > > > > So I’m proposing to deprecate it but still keep it for backward compatibility for now (probably a full cycle). > > > > > > > > WDYT? > > > > > > Note that I’d like to change a bit the proposal and instead of making the exception available from a script service, I’d prefer to make it available as a known velocity binding such as $lastException. There’s no reason to use a script service since that would mean it would work for all scripts and in this case we only want it to work for Velocity. > > > > > > Since there’s no way to get the Velocity Context from within an uberspector, I’ll get it by using our Component Manager and get the VelocityManager component and call getVelocityContext()… If you know a better way, let me know. > > > > hmm… this would mean that I’d need to put this new uberspector in xwiki-platform since VelocityManager is in platform ATM… (@Thomas: our discussion of yesterday ;)). > > > There’s an alternative, which is to modify our implementation of VelocityEngine.evaluate() and decorate the source with a #try() directive so that it’s always called (and make sure that calling it nested won’t affect it for backward compat). > > This could be simpler to implement and doesn’t force us to move some velocity code to platform. > > WDYT? > > Thanks > -Vincent > > [snip]

On 15 Jan 2016 at 13:26:08, vincent(a)massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote: Actually forget that, I don’t need to use VelocityManager in practice (and it would be costly to do so). I just need to use the Execution component: VelocityContext vcontext = (VelocityContext) this.execution.getContext().getProperty( VelocityExecutionContextInitializer.VELOCITY_CONTEXT_ID); Thanks -Vincent Ps: Sorry for the live design noise... > > Thanks > -Vincent > > > Thanks > > -Vincent > > > > > > On 15 Jan 2016 at 12:15:41, vincent(a)massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote: > > > > > > > > [snip] > > > > > > > > > > > >> > Hi devs, > > > > > > > > >> > > > > > > > > > >> > Right now our strategy is for script services and script APIs in general > > > > > > > > >> > to catch exceptions, store them and offer a getLastError() method to get > > > > > > > > >> > them (see > > > > > > > > >> > http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra… > > > > > > > > >> > ) > > > > > > > > >> > > > > > > > > > >> > However it would be much nicer to: > > > > > > > > >> > * Let our script services generate exceptions > > > > > > > > >> > * Offer a velocity script service to get the last exception raised by a > > > > > > > > >> > java call from velocity > > > > > > > > >> > * Implement this uberspector to catch the exceptions and to set them in > > > > > > > > >> > the execution context > > > > > > > > >> > > > > > > > > > >> > That should be quite easy to implement IMO. > > > > > > > > >> > > > > > > > > > >> > WDYT? > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> +1, it's a pain to call setLastError() everywhere there can be an exception > > > > > > > > >> thrown, and we almost always forget to do it (for this reason). > > > > > > > > >> > > > > > > > > >> Note that we also have the #try() directive now. > > > > > > > > > > > > > > > > > > Yes, I should have mentioned that there’s indeed also this possibility: > > > > > > > > > * Have script API throw Exceptions > > > > > > > > > * Force velocity script users to wrap their code with the try directive when they need to catch exceptions > > > > > > > > > > > > > > > > > > I still believe that the use of the Exception-catching uberspector is better. > > > > > > > > > > > > > > > > > > WDYT? > > > > > > > > > > > > > > > > Does it mean you plan to get rid of new #try directive ? Because it > > > > > > > > will be broken with this new uberspector. > > > > > > > > > > > > > > That’s a good point, I had not thought about the implementation at this stage. > > > > > > > > > > > > > > I think this could still work. When the #try directive is used I’d just have to setup some flag somewhere in Velocity and in the uberspector I could check if this flag is set and if so then don’t catch the exception. > > > > > > > > > > > > > > > > > > Actually, thinking more, I think you’re right and that the #try directive plays exactly the same role as an Exception-catching uberspector and I don’t see the need for the #try directive if we provide an uberspector. > > > > > > > > > > > > So I’m proposing to deprecate it but still keep it for backward compatibility for now (probably a full cycle). > > > > > > > > > > > > WDYT? > > > > > > > > > > Note that I’d like to change a bit the proposal and instead of making the exception available from a script service, I’d prefer to make it available as a known velocity binding such as $lastException. There’s no reason to use a script service since that would mean it would work for all scripts and in this case we only want it to work for Velocity. > > > > > > > > > > Since there’s no way to get the Velocity Context from within an uberspector, I’ll get it by using our Component Manager and get the VelocityManager component and call getVelocityContext()… If you know a better way, let me know. > > > > > > > > hmm… this would mean that I’d need to put this new uberspector in xwiki-platform since VelocityManager is in platform ATM… (@Thomas: our discussion of yesterday ;)). > > > > > > > > > There’s an alternative, which is to modify our implementation of VelocityEngine.evaluate() and decorate the source with a #try() directive so that it’s always called (and make sure that calling it nested won’t affect it for backward compat). > > > > > > This could be simpler to implement and doesn’t force us to move some velocity code to platform. > > > > > > WDYT? > > > > > > Thanks > > > -Vincent > > > > > > [snip] > > _______________________________________________ > devs mailing list > devs(a)xwiki.org > http://lists.xwiki.org/mailman/listinfo/devs

I need to think more about this because it means velocity macros won’t show an error box anymore in case of error…. Thanks -Vincent On 15 Jan 2016 at 17:20:21, vincent(a)massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote: _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

vincent(a)massol.net <mailto:vincent@massol.net> 18 January 2016 at 11:03 Hi devs, After a lot of thinking and experimentation (see the thread’s details), I have found that this first proposal is not a good idea. I’m thus proposing to replace it with the following best practice: * Let our script services generate exceptions * If the velocity scripts with to handle the exceptions, then they should use the #try() directive. If they don’t want to, they don’t have to do anything since the MacroTransformation or the template (contentvars.vm for example) will catch it and display it to the user. More precisely I’m proposing that: * Existing Script APIs in Java should not be modified as that would break backward compatibility. New signatures can be added and old one deprecated and moved to the legacy modules. After new signatures have been introduced, existing velocity scripts can be updated to use the new signatures and to use the #try directive if needed. * New Script APIs must use the new best practices (if agreed :)), i.e. throw Exceptions, and new velocity scripts must use the #try() directive if they need to handle exceptions. WDYT? Thanks -Vincent _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs vincent(a)massol.net <mailto:vincent@massol.net> 14 January 2016 at 17:51 Hi devs, Right now our strategy is for script services and script APIs in general to catch exceptions, store them and offer a getLastError() method to get them (see http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra…) However it would be much nicer to: * Let our script services generate exceptions * Offer a velocity script service to get the last exception raised by a java call from velocity * Implement this uberspector to catch the exceptions and to set them in the execution context That should be quite easy to implement IMO. WDYT? Thanks -Vincent PS: This is http://jira.xwiki.org/browse/XWIKI-2374 _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

Our current strategy has been to keep legacy forever, see: http://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HBackwar… Our idea was to start not bundling legacy jars by default but we need the EM to support legacy jars first as otherwise users will see breakages when they install extensions requiring legacy APIs. Do you have a different idea?

On 18 Jan 2016, at 11:03, vincent(a)massol.net wrote: Hi devs, After a lot of thinking and experimentation (see the thread’s details), I have found that this first proposal is not a good idea. I’m thus proposing to replace it with the following best practice: * Let our script services generate exceptions * If the velocity scripts with to handle the exceptions, then they should use the #try() directive. If they don’t want to, they don’t have to do anything since the MacroTransformation or the template (contentvars.vm for example) will catch it and display it to the user. More precisely I’m proposing that: * Existing Script APIs in Java should not be modified as that would break backward compatibility. New signatures can be added and old one deprecated and moved to the legacy modules. After new signatures have been introduced, existing velocity scripts can be updated to use the new signatures and to use the #try directive if needed. * New Script APIs must use the new best practices (if agreed :)), i.e. throw Exceptions, and new velocity scripts must use the #try() directive if they need to handle exceptions. WDYT? Thanks -Vincent On 14 Jan 2016 at 17:51:04, vincent(a)massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote: > Hi devs, > > Right now our strategy is for script services and script APIs in general to catch exceptions, store them and offer a getLastError() method to get them (see http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra…) > > However it would be much nicer to: > * Let our script services generate exceptions > * Offer a velocity script service to get the last exception raised by a java call from velocity > * Implement this uberspector to catch the exceptions and to set them in the execution context > > That should be quite easy to implement IMO. > > WDYT? > > Thanks > -Vincent > > PS: This is http://jira.xwiki.org/browse/XWIKI-2374

+1 for throwing exceptions in script services and using #try to catch them if we want. Thanks, Marius On Thu, Mar 31, 2016 at 3:17 PM, Vincent Massol &lt;vincent(a)massol.net&gt; wrote: _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

So far we have the following devs who agree: - thomas - marius - vincent What about Edy, Sergiu and the others? Thanks -Vincent _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

Sure, that’s the point. The scripts can decide to catch or not (ie. to handle or not). If not then it’s caught anyway at the level of MacroTransformation (for macros) or at the level of contentview.vm. In the majority of cases there isn’t much that the script can do and it shouldn’t catch anything (it should catch only if it can handle it, i.e. do something with the exception). Thanks -Vincent details), I have found that this first proposal is not a good idea. I’m thus proposing to replace it with the following best practice: should use the #try() directive. If they don’t want to, they don’t have to do anything since the MacroTransformation or the template (contentvars.vm for example) will catch it and display it to the user. break backward compatibility. New signatures can be added and old one deprecated and moved to the legacy modules. After new signatures have been introduced, existing velocity scripts can be updated to use the new signatures and to use the #try directive if needed. i.e. throw Exceptions, and new velocity scripts must use the #try() directive if they need to handle exceptions. (mailto:vincent@massol.net)) wrote: general to catch exceptions, store them and offer a getLastError() method to get them (see http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra… ) by a java call from velocity in the execution context _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

On 03 Apr 2016, at 10:49, Thomas Mortagne &lt;thomas.mortagne(a)xwiki.com&gt; wrote: You will start catching when you want to deal with errors (before that the Velocity macro will deal with it for you automatically) and it does not really cost much more lines to deal with errors that way than using checking for null return (for method that actually return something that could be null to indicate errors) and getLastError() concept like we do now.

On Sun, Apr 3, 2016 at 12:19 AM, Eduard Moraru &lt;enygma2002(a)gmail.com&gt; wrote: > +0 > > I can`t help but feel we are stretching Velocity more than what it was > built for and we might end up with a more verbose and spaghetti code than > what we would have liked (and than we currently have). > > As long as we try not to abuse it, I guess we can see how it goes. > > Thanks, > Eduard > > On Sat, Apr 2, 2016 at 11:59 AM, Vincent Massol &lt;vincent(a)massol.net&gt; wrote: > >> >>> On 01 Apr 2016, at 23:04, Sergiu Dumitriu &lt;sergiu(a)xwiki.org&gt; wrote: >>> >>> I think that it's a good idea for script services to throw somewhat >>> expected exceptions, signalling invalid usage attempts (user not >>> authorized, wrong arguments...) that would then be caught in Velocity. >>> But deeper platform issues (DB errors, unexpected NPE, OOM...) should be >>> handled outside the user's code itself, at the skin level (view.vm). >> >> Sure, that’s the point. The scripts can decide to catch or not (ie. to >> handle or not). If not then it’s caught anyway at the level of >> MacroTransformation (for macros) or at the level of contentview.vm. In the >> majority of cases there isn’t much that the script can do and it shouldn’t >> catch anything (it should catch only if it can handle it, i.e. do something >> with the exception). >> >> Thanks >> -Vincent >> >>> >>> On 04/01/2016 05:45 AM, Vincent Massol wrote: >>>> So far we have the following devs who agree: >>>> - thomas >>>> - marius >>>> - vincent >>>> >>>> What about Edy, Sergiu and the others? >>>> >>>> Thanks >>>> -Vincent >>>> >>>>> On 31 Mar 2016, at 14:17, Vincent Massol &lt;vincent(a)massol.net&gt; wrote: >>>>> >>>>> Guys, I’d like that we progress on this. >>>>> >>>>> I didn’t get any agreement or disagreement to this proposal. >>>>> >>>>> Any take? >>>>> >>>>> Thanks >>>>> -Vincent >>>>> >>>>> >>>>>> On 18 Jan 2016, at 11:03, vincent(a)massol.net wrote: >>>>>> >>>>>> Hi devs, >>>>>> >>>>>> After a lot of thinking and experimentation (see the thread’s >> details), I have found that this first proposal is not a good idea. I’m >> thus proposing to replace it with the following best practice: >>>>>> >>>>>> * Let our script services generate exceptions >>>>>> * If the velocity scripts with to handle the exceptions, then they >> should use the #try() directive. If they don’t want to, they don’t have to >> do anything since the MacroTransformation or the template (contentvars.vm >> for example) will catch it and display it to the user. >>>>>> >>>>>> More precisely I’m proposing that: >>>>>> >>>>>> * Existing Script APIs in Java should not be modified as that would >> break backward compatibility. New signatures can be added and old one >> deprecated and moved to the legacy modules. After new signatures have been >> introduced, existing velocity scripts can be updated to use the new >> signatures and to use the #try directive if needed. >>>>>> * New Script APIs must use the new best practices (if agreed :)), >> i.e. throw Exceptions, and new velocity scripts must use the #try() >> directive if they need to handle exceptions. >>>>>> >>>>>> WDYT? >>>>>> >>>>>> Thanks >>>>>> -Vincent >>>>>> >>>>>> >>>>>> On 14 Jan 2016 at 17:51:04, vincent(a)massol.net (vincent(a)massol.net >> (mailto:vincent@massol.net)) wrote: >>>>>> >>>>>>> Hi devs, >>>>>>> >>>>>>> Right now our strategy is for script services and script APIs in >> general to catch exceptions, store them and offer a getLastError() method >> to get them (see >> http://extensions.xwiki.org/xwiki/bin/view/Extension/Script+Module#HBestPra… >> ) >>>>>>> >>>>>>> However it would be much nicer to: >>>>>>> * Let our script services generate exceptions >>>>>>> * Offer a velocity script service to get the last exception raised >> by a java call from velocity >>>>>>> * Implement this uberspector to catch the exceptions and to set them >> in the execution context >>>>>>> >>>>>>> That should be quite easy to implement IMO. >>>>>>> >>>>>>> WDYT? >>>>>>> >>>>>>> Thanks >>>>>>> -Vincent >>>>>>> >>>>>>> PS: This is http://jira.xwiki.org/browse/XWIKI-2374