On 15 Jan 2016 at 13:23:07, vinc...@massol.net (vinc...@massol.net(mailto:vinc...@massol.net)) wrote:
> 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. Actually VelocityManager is already in commons, it’s the impl that is in platform. So right now it will work for us; it just won’t work for users who use just commons. Like if you use the Rendering in standalone mode. Thanks -Vincent > Thanks > -Vincent > > > On 15 Jan 2016 at 12:15:41, vinc...@massol.net > (vinc...@massol.net(mailto:vinc...@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#HBestPractices > > > > > > > >> > ) > > > > > > > >> > > > > > > > > >> > 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@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs
