Hi Edy,
On 1 Oct 2014 at 12:45:52, Eduard Moraru
([email protected](mailto:[email protected])) wrote:
> Hi,
>
> Interesting, though I do have my worries about having to deal with actual
> exceptions in velocity. Probably the most common usecase would be just to
> display the exception, but others may now be tempted to look at exception
> types and display custom messages for each error situation. That should
> improve the quality of the UIs, but at the same time I feel like it will
> increase the amount of velocity code in wiki pages.
>
> Generally, I`m +1 since I had to deal on some occasions with exception
> handling in velocity and found myself re-coding over and over again the
> methanism of passing the exception from the script service to the velocity
> UI.
>
> However, I feel there are some things that are could be done to improve it
> in practice:
> 1. Make sure that the "try" directive clears any existing context exception
> before executing it's content and possibly setting a new exception in the
> context. Example:
> #try()
>
> #end
>
> #try()
>
> #end
I was going to say that it’s already the case but then I checked the code and
indeed I changed the implementation when I moved to the Directive approach
suggested by Sergiu and I forgot to clear it. It’s done now.
> 2. Support nested exception handling. Example:
> #try()
>
> #try()
>
> #end
>
> #end
>
> I agree that this point (2.) could probably be solved by (1.) if the "try"
> directive clears the context exception *after* executing the code so that
> any leftovers from nested exceptions are cleared.
>
> 2.1. However, my initial suggestion was to maybe support "named exceptions"
> (i.e. a parameter for the "try" directive that could specify the variable
> where to expose the exception). Example:
> #try()
>
> #try("nestedException") <-- or #try($nestedException) but I think this
> is more problematic since it really has to be set to $NULL before
>
> #end
>
> #end
>
>
> 2.2 Thinking of the above and the nested usecase, I just wanted to mention
> a possible #throw($exception) directive in case we want to use the main
> "try" block as a catch-all handler, even if nested try blocks caught and
> reported errors as they occurred (so they would now bubble the exception
> towards the main try block to signal that the operation as a whole was
> problematic). Example:
> #try()
>
> #try()
>
> #end
> #if ($exception)
>
> #throw($exception) <-- this probably stops the execution here.
> #end
> #end
>
> Of course, this can be done with a variable instead in the exception
> handling of each nested try blocks. Example:
> #try()
>
> #try()
>
> #end
> #if ($exception)
>
> #set($failed=true) <-- this leaves the developer to decide by using
> some IFs.
> #end
> #end
> > considering the $failed variable>
>
> Point 1. is a must IMO to avoid unwanted errors. The rest is just food for
> thought, perhaps you consider them useful as well.
Yes, good points that we can review after using it for real in some code.
Point 1 is fixed now.
Thanks
-Vincent
>
> Thanks,
> Eduard
>
>
> On Fri, Sep 19, 2014 at 3:48 PM, [email protected]
> wrote:
>
> >
> >
> >
> >
> > On 19 Sep 2014 at 10:24:51, Thomas Mortagne ([email protected]
> > (mailto:[email protected])) wrote:
> >
> > > #try
> > >
> > > was not possible ? (without parenthesis)
> >
> > AFAIK it’s not possible since BLOCK directives in Velocity need
> > parenthesis.
> >
> > Thanks
> > -Vincent
> >
> > > On Fri, Sep 19, 2014 at 12:04 AM, [email protected] wrote:
> > > >
> > > >
> > > > On 18 Sep 2014 at 19:41:56, [email protected] ([email protected]
> > (mailto:[email protected])) wrote:
> > > >
> > > >> Hi Sergiu,
> > > >>
> > > >> On 18 Sep 2014 at 19:29:52, Sergiu Dumitriu ([email protected](mailto:
> > [email protected])) wrote:
> > > >>
> > > >> > 1. I would rather add proper directives instead of macros and
> > > >> > uberspector hacks: http://stackoverflow.com/q/159292/620249
> > > >>
> > > >> Argh… I wanted that and I asked Thomas and he told me it wasn’t
> > possible and I didn’t double check. I should have since I’ve now finished
> > my implementation and redoing it as a directive means dumping what I’ve
> > done a third time in the end. Anyway I agree that a directive is better so
> > I’ll redo it.
> > > >
> > > > I’ve now implemented a directive and it’s so cool :)
> > > >
> > > > It’s so simple compared to what I had, I love it.
> > > >
> > > > Here’s the usage code now:
> > > >
> > > > #try()
> > > > #set($outputSyntax =
> > $xwiki.getAvailableRendererSyntax($request.outputSyntax,
> > $request.outputSyntaxVersion))
> > > > #if ($outputSyntax)
> > > > ## If the passed syntax is not an HTML-compatible syntax we need to
> > HTML-escape it so that it can be
> > > > ## displayed fine in HTML (since at the point this vm file is called
> > we're already inside an HTML page with
> > > > ## panels on the side, header, etc).
> > > > #set($syntaxType = $outputSyntax.type.toIdString())
> > > > #if (($syntaxType == "xhtml") || ($syntaxType == "html"))
> > > > #set ($renderedContent = $tdoc.getRenderedContent($outputSyntax))
> > > > #else
> > > > ## Make sure to print correctly the result when it's not HTML
> > > > #set ($renderedContent = "
> > $escapetool.html($tdoc.getRenderedContent($outputSyntax))
> > ")
> > > > #end
> > > > #else
> > > > #set ($renderedContent = $tdoc.getRenderedContent())
> > > > #end
> > > > #end
> > > > …
> > > >
> >
> > > > #if ("$!exception" != '')
> > > > #displayException($exception)
> > > > #else
> > > > $renderedContent
> > > > #end
> > > >
> >
> > > > …
> > > >
> > > > I like it so much that I’m going to commit it and if someone doesn’t
> > agree I’ll revert the commit :)
> > > >
> > > > Thanks Sergiu
> > > > -Vincent
> > > >
> > > >> > 2. So far it has been the job of our scriptable wrappers to catch
> > > >> > exceptions, does this proposal mean that:
> > > >> > a. We should stop doing that and write try-catch logic in our
> > > >> > templates/scripts? IOW, catching exceptions in wrappers is a bad
> > design
> > > >> > pattern?
> > > >>
> > > >> I’ve not answered this question because I don’t know yet. I think
> > we’ll discover that as we start using the new try/catch. ATM I’m only
> > planning to use it in contentview.vm.
> > > >>
> > > >> > b. We didn't do that enough, and as a workaround we're adding
> > exception
> > > >> > handling in Velocity?
> > > >>
> > > >> Yes, that’s true. We now have both cases in our code (code throwing
> > exceptions, like a lot of methods in Document, XWiki) and others catching
> > exceptions.
> > > >>
> > > >> This proposals offers a finer-grained control in vm while not
> > breaking backward compatibility. Any change in our script APIs would break
> > compatibility (this is what I started doing before realizing it would break
> > too many things)…
> > > >>
> > > >> FTR here’s what I have so far in term of usage, in contentview.vm:
> > > >>
> > > >> #try()
> > > >> #set($outputSyntax =
> > $xwiki.getAvailableRendererSyntax($request.outputSyntax,
> > $request.outputSyntaxVersion))
> > > >> #if ($outputSyntax)
> > > >> ## If the passed syntax is not an HTML-compatible syntax we need to
> > HTML-escape it so that it can be
> > > >> ## displayed fine in HTML (since at the point this vm file is called
> > we're already inside an HTML page with
> > > >> ## panels on the side, header, etc).
> > > >> #set($syntaxType = $outputSyntax.type.toIdString())
> > > >> #if (($syntaxType == "xhtml") || ($syntaxType == "html"))
> > > >> #set ($renderedContent = $tdoc.getRenderedContent($outputSyntax))
> > > >> #else
> > > >> ## Make sure to print correctly the result when it's not HTML
> > > >> #set ($renderedContent = "
> > > > $escapetool.html($tdoc.getRenderedContent($outputSyntax))
> > > > ")
> > > >> #end
> > > >> #else
> > > >> #set ($renderedContent = $tdoc.getRenderedContent())
> > > >> #end
> > > >> #catch()
> > > >>
> > > >> …
> > > >>
> > > >
> > > >> #if ("$!exception" != '')
> > > >> #displayException($exception)
> > > >> #else
> > > >> $renderedContent
> > > >> #end
> > > >>
> > > >
> > > >> …
> > > >>
> > > >>
> > > >> Thanks
> > > >> -Vincent
> > > >>
> > > >> > On 09/18/2014 09:28 AM, [email protected] wrote:
> > > >> > > Hi Devs,
> > > >> > >
> > > >> > > Example of a use case:
> > > >> > >
> > > >> > > * In contentview.vm we need to be able to catch when
> > getRenderedContent() throws an exception in order to be able to still
> > display the page title, content menu and last modification line. We want
> > only the content part to display an error.
> > > >> > >
> > > >> > > I’ve tried implementing the following:
> > > >> > > * Modified Document.getRenderedContent() to not throw exceptions
> > any more
> > > >> > > * I’ve added Document.setError() and getError()
> > > >> > > * Then added a #displayRenderedContent($renderedContent)
> > velocimacro to display an error, calling Document.getError()
> > > >> > >
> > > >> > > However that’s no good because we have lots of places in our code
> > (and in extensions we don’t control) that call getRenderedContent() and
> > they expect the whole page rendering to fail in case of an error…
> > > >> > >
> > > >> > > Thus after discussing quickly on IRC and with Thomas we’d like to
> > propose instead:
> > > >> > >
> > > >> > > * Add a new #try() velocimacro that would push a new empty List
> > in the Execution Context to a Stack variable
> > > >> > > * Add a new uberspector that would, based on this flag do a
> > try/catch around the method called (can be done easily by returning a
> > custom VelMethod)
> > > >> > > * The uberspector catch would add the exception to the List in
> > the Execution Context
> > > >> > > * Add a new #catch() velocimacro that would pop from the Stack in
> > the Execution Context and that would set 2 variables in the Velocity
> > Context:
> > > >> > > ** $exception: the first exception
> > > >> > > ** $exceptions: the list of all exceptions that happened between
> > the #try() and #catch()
> > > >> > >
> > > >> > > The good part with this is:
> > > >> > > * No change in behavior to all existing code
> > > >> > > * Code that want to do something in case of error can do it using
> > #try()/#catch() for java methods throwing exceptions (like
> > getRenderedContent())
> > > >> > >
> > > >> > > Of course for the future we would need to decide if script
> > services should stop doing try/catch or not but that’s another debate I’d
> > prefer to separate from this thread.
> > > >> > >
> > > >> > > WDYT?
> > > >> > >
> > > >> > > Thanks
> > > >> > > -Vincent
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
