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, 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
