Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #219 (closed enhancement: duplicate)

Opened 14 years ago

Last modified 12 years ago

Even better better error handling

Reported by: kevin Owned by: anonymous
Priority: normal Milestone: 0.9
Component: TurboGears Version:
Severity: normal Keywords:
Cc: wkurdziolek@…

Description (last modified by kevin) (diff)

In 0.9, the error handling has moved away from the validation_error method, which is very inconvenient to use. As of this writing, validation errors are checked like this:

    @turbogears.expose(validators=[...])
    def mymethod(self, **kw):
        if cherrypy.request.form_errors:
            ...do something for the bad case...
        ...handle the good case...

This works, but it doesn't read quite as nicely. I propose this extension:

    @turbogears.expose(validators=[...])
    def mymethod(self, **kw):
        handle the good case

    def mymethod_error(self, errors, **kw):
        handle the bad case

The plus to this arrangement is that it feels more like a try/except block. The good case comes first, and the bad case has its own block down below.

Also, in order to help with code reuse, the notion of this came up:

    def tackle_errors(self, errors, **kw):
        handle the bad case

    @turbogears.expose(validators=[...], error_handler=self.tackle_errors)
    def mymethod(self, **kw):
        handle the good case

error_handler is optional. If set, that's the method that gets called.

It's also worth noting that the error handling methods are passed the errors and all of the same arguments that would be passed to the original method.

These ideas came up in an off-list conversation with Lucas Nelson.

Change History

comment:1 Changed 14 years ago by kevin

  • Description modified (diff)

comment:2 Changed 14 years ago by Jorge Godoy <jgodoy@…>

Some way to define a global error method would also be interesting. For example, I'd like having something to set a tg_flash message and calling the same page again. Other people might want to highlight the wrong input and add an error message to them (à la what is done with Formulator in Plone/Zope?).

I also believe that in a production environment, the exception shouldn't be raised, but should go to server.log and the user should be presented with a default "Oops, something has gone bad! It is XX:YY:ZZ hours and you should inform the site maintainer of this error. He'll know what to do. Meanwhile, we're sorry and suggest you try again after pressing the back button of your browser." message (not exactly this one, but with enough information to help the developer getting the right traceback).

For the development environment it is okay having the traceback on screen.

comment:3 Changed 14 years ago by simon.belak@…

Magic names are evil. Wouldn't it be better to have a decorator to explicitly declare a function an error handler (of a given exposed method):

@turbogears.expose(validators=[...])
def mymethod(self, **kw):
    handle the good case

@error(mymethod)
def mymethod_error(self, errors, **kw):
    handle the bad case

error decorator would of course allow composition and therefore reusability as outlined in the second proposition.

error without an argument could denote a global error handler as proposed by Jorge.

comment:4 Changed 14 years ago by michele

I have to agree regarding magic names, IMHO we should keep it explicit as python itself aims to do.

I like the decorator solution, maybe it's better to call it @error_handler? anyway is this doable?

Also, another opinion on this by Dan Jacob:

1. An error_handler argument to @turbogears.expose.

@turbogears.expose(html="templates.form")
def add(self):
     return dict(form=article_form())

@turbogears.expose(inputform=article_form(), error_handler=add)
     # go straight to adding an article and redirect, no need to check
for errors 

comment:5 Changed 14 years ago by Simon Belak <simon.belak@…>

My take on decorator error_handler (functionality as described above):

from dispatch import generic, strategy

...

@generic()
def _dispatch_error(error_source, errors, **kw):
	pass

def error_handler(error_source=None):
	if error_source:
		when = "error_source == %s" % error_source.__name__
	else:
		when = strategy.default

	def register(func):

		def normalize(func):
			def prepend_arg(error_source, errors, **kw):
				return func(errors, **kw)
			return prepend_arg

		_dispatch_error.when(when)(normalize(func))
		return func

	return register

Only other change needed in controllers.py is to call _dispatch_error either in expose() or in _execute_func() and add error_handler to all.

If this is what you were looking for, I would be glad to a write a full-on patch for controllers.py.

comment:6 Changed 14 years ago by Simon Belak <simon.belak@…>

In retrospective, to keep it more flexible _dispatch_error() should take positional arguments as well.

comment:7 Changed 14 years ago by Simon Belak <simon.belak@…>

Have submited a patch as #258.

comment:8 Changed 13 years ago by wkurdziolek@…

  • Cc wkurdziolek@… added

I'd like to see error handling available on widgets, too. This would be incredibly helpful when composing forms from common widgets (including forms) b/c it would allow one to automatically compose error handling, as well, rather than manually composing error handling methods for each method that uses a different form. This may not seem very valuable w/ a form of 10 or less widgets, but I've been a developer on several projects where forms are composed of several reusable forms of 5 to 15 widgets. The only way to sanely manage error handling was to use per-widget error handling.

Implementation-wise, I don't know if it's better to allow specifying the error handler to the widget constructor or to make it an overloadable method on the widget class. Also it doesn't sound like what I'm proposing would necessarily be mutually exclusive w/ the ideas that have already been proposed here.

comment:9 Changed 13 years ago by kevin

  • Status changed from new to closed
  • Resolution set to duplicate

This ticket is dead in favor of #258. Any further discussion should take place there or on-list.

Note: See TracTickets for help on using tickets.