On 15 Jan 2016 at 07:24:35, Marius Dumitru Florea (mariusdumitru.florea@xwiki.com(mailto:mariusdumitru.florea@xwiki.com)) wrote:

On Thu, Jan 14, 2016 at 6:51 PM, 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#HBestPrac... )

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? Thanks -Vincent

Thanks, Marius

...

Thanks -Vincent

PS: This is http://jira.xwiki.org/browse/XWIKI-2374

  On 15 Jan 2016 at 10:34:58, Thomas Mortagne (thomas.mortagne@xwiki.com(mailto:thomas.mortagne@xwiki.com)) wrote:

On Fri, Jan 15, 2016 at 9:18 AM, vincent@massol.net wrote:

...

On 15 Jan 2016 at 07:24:35, Marius Dumitru Florea (mariusdumitru.florea@xwiki.com(mailto:mariusdumitru.florea@xwiki.com)) wrote:

...

On Thu, Jan 14, 2016 at 6:51 PM, 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#HBestPrac... )

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. Thanks -Vincent

...

Thanks -Vincent

...

Thanks, Marius

...

Thanks -Vincent

PS: This is http://jira.xwiki.org/browse/XWIKI-2374

  On 15 Jan 2016 at 11:55:26, vincent@massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote:

On 15 Jan 2016 at 10:38:05, vincent@massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote:

...

On 15 Jan 2016 at 10:34:58, Thomas Mortagne (thomas.mortagne@xwiki.com(mailto:thomas.mortagne@xwiki.com)) wrote:

...

On Fri, Jan 15, 2016 at 9:18 AM, vincent@massol.net wrote:

...

On 15 Jan 2016 at 07:24:35, Marius Dumitru Florea (mariusdumitru.florea@xwiki.com(mailto:mariusdumitru.florea@xwiki.com)) wrote:

...

On Thu, Jan 14, 2016 at 6:51 PM, 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#HBestPrac... )

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. Thanks -Vincent

Thanks -Vincent

...

Thanks -Vincent

...

...

Thanks -Vincent

...

Thanks, Marius

...

Thanks -Vincent

PS: This is http://jira.xwiki.org/browse/XWIKI-2374

[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#HBestPrac... > > ) > > > > 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:23:07, vincent@massol.net (vincent@massol.net(mailto:vincent@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, vincent@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#HBestPrac... > >> > ) > >> > > >> > 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]

Ok I’ve finished the implementation and will be committing soon. Note that this could change slightly what the user sees. For example in the past if you had this velocity script: {{velocity}} $someclass.someMethodThrowingException() … rest of the script here... {{/velocity}} The user would see a stack trace on his screen before upgrading to 8.0 and after upgrading to 8.0 he’ll see something different which depends on what the “… rest of the script here…” does. What I don’t like too much is that this could lead to unwanted side effects. For example, imagine that we have the following: {{velocity}} #set ($docReference = $someclass.someMethodComputingTheReferenceButThrowingException()) … $xwiki.getDocument($docReference).save() ... {{/velocity}} Then if an exception is raised in someMethodComputingTheReferenceButThrowingException() it used to display an exception but will now do something else. It happens that in this case it’ll report a NPE (AFAICS in the code) but we could imagine it would create a wrong document in the wiki, etc. Any thoughts about this? Should I still go ahead (I think so). Thanks -Vincent On 15 Jan 2016 at 13:37:39, vincent@massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote:

On 15 Jan 2016 at 13:26:08, vincent@massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote:

...

On 15 Jan 2016 at 13:23:07, vincent@massol.net (vincent@massol.net(mailto:vincent@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.

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@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#HBestPrac... > > > >> > ) > > > >> > > > > >> > 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

What about making possible to enable/disable as a {{velocity}} macro parameter (disabled by default) ? On Fri, Jan 15, 2016 at 5:41 PM, vincent@massol.net <vincent@massol.net> wrote:

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@massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote:

...

Ok I’ve finished the implementation and will be committing soon.

Note that this could change slightly what the user sees.

For example in the past if you had this velocity script:

{{velocity}} $someclass.someMethodThrowingException() … rest of the script here... {{/velocity}}

The user would see a stack trace on his screen before upgrading to 8.0 and after upgrading to 8.0 he’ll see something different which depends on what the “… rest of the script here…” does.

What I don’t like too much is that this could lead to unwanted side effects. For example, imagine that we have the following:

{{velocity}} #set ($docReference = $someclass.someMethodComputingTheReferenceButThrowingException()) … $xwiki.getDocument($docReference).save() ... {{/velocity}}

Then if an exception is raised in someMethodComputingTheReferenceButThrowingException() it used to display an exception but will now do something else. It happens that in this case it’ll report a NPE (AFAICS in the code) but we could imagine it would create a wrong document in the wiki, etc.

Any thoughts about this? Should I still go ahead (I think so).

Thanks -Vincent

On 15 Jan 2016 at 13:37:39, vincent@massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote:

...

On 15 Jan 2016 at 13:26:08, vincent@massol.net (vincent@massol.net(mailto:vincent@massol.net)) wrote:

...

On 15 Jan 2016 at 13:23:07, vincent@massol.net (vincent@massol.net(mailto:vincent@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.

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@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#HBestPrac... > > > > > >> > ) > > > > > >> > > > > > > >> > 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

_______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Thomas Mortagne

Vincent, is there not a need to version the legacy? I know of no installation that does not have the legacy jars and I wonder if this could not be better controlled so that installers can actively get rid of the legacy. paul

vincent@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@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs vincent@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#HBestPrac...)

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@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

vincent@massol.net wrote:

...

wonder if this could not be better controlled so that installers can

...

actively get rid of the legacy.

Our current strategy has been to keep legacy forever, see: http://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HBackward...

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? Yes, I meant that legacy jars should give the feeling that one should get rid of them. Marking them with "legacy of version 5.3" (which would mean something such as "abandadonned at 5.3" would let the installers be aware that the software they run still needs legacy that is maybe 2 years old.

paul

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@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@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#HBestPrac...)

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 it certainly makes writing script services much easier (and harder to ignore proper error handling on script side :)) On Thu, Mar 31, 2016 at 3:20 PM, Marius Dumitru Florea <mariusdumitru.florea@xwiki.com> wrote:

+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 <vincent@massol.net> 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@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@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#HBestPrac... )

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@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

_______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Thomas Mortagne

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). 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 <vincent@massol.net> 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@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@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#HBestPrac...)

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@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Sergiu Dumitriu http://purl.org/net/sergiu

+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 <vincent@massol.net> wrote:

...

On 01 Apr 2016, at 23:04, Sergiu Dumitriu <sergiu@xwiki.org> 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 <vincent@massol.net> 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@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@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#HBestPrac... )

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@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

-- Sergiu Dumitriu http://purl.org/net/sergiu _______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

_______________________________________________ devs mailing list devs@xwiki.org http://lists.xwiki.org/mailman/listinfo/devs

On 03 Apr 2016, at 10:49, Thomas Mortagne <thomas.mortagne@xwiki.com> 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.

Yes, and in addition it will prevent side effects. Right now if you call a method than can fail and you don’t check the result, the next lines will be wrong and can even cause problems. For example: {{velocity}} #set ($someVar = $someobject.somethod()) ## result of server-side computation that failed and return null #set ($somedoc = $xwiki.getDocument("${someVar}-whatever”)) $somedoc.save() {{/velocity}} If someobject.somemethod() throws an Exception then there’s no risk of causing side effects. Thanks -Vincent

On Sun, Apr 3, 2016 at 12:19 AM, Eduard Moraru <enygma2002@gmail.com> 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 <vincent@massol.net> wrote:

...

...

On 01 Apr 2016, at 23:04, Sergiu Dumitriu <sergiu@xwiki.org> 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 <vincent@massol.net> 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@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@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#HBestPrac... ) >> >> 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