Keep up the good work.
The problem is, there's no explicit condition in a try/except block.
Any statement after try before except may throw an exception that may
or may not be caught by the except clause. It's less clear how the
program will actually run than if/else.
Also, sometime in the future Python may not support raising classes
that are not derived from Exception (I read that somewhere, but can't
remember where--I think it was a PEP)--that would mean the TG raise
redirect would not work. Not to mention, it doesn't seem very Pythonic
to be raising some class that is not an error--it's just not the one
obvious way.
Finally, although I think it's a bad idea to use this as the sole
reason for ruling something out, raising/catching an exception is less
performant than using a simple if/else due to the overhead of the
exception handling mechanism.
On the other hand, I think HTTPRedirect exception in
cherrypy/Turbogears is a good use of it, in python that is. Web apps is
some special kind of programs that fits with GOTOs and this is actually
goto.
That's amazing--fixed in less than 12 hours after my review was posted!
I'll update the review tonight.
> As for the raise HTTPRedirect construct
>
> I'd also be fine with
>
> return cherrypy.HTTPRedirect(...)
Yes, I like that too.
> The nice thing about raise is something like this:
>
> @turbogears.expose()
> def mypublicmethod(self, arg):
> someval = self.dosomething(arg)
> return self.process(someval)
>
> def dosomething(self, arg):
> # oops, something isn't in the right state...
> raise cherrypy.HTTPRedirect(...)
>
> Of course, that is an exceptional condition, so maybe that makes perfect sense.
Let's think about what the "dosomething" method is doing here--it's
creating something to be processed, or short-circuiting to a redirect.
IMHO that type of control flow leads to buggy apps because you're never
quite sure what to expect when you call a method--the control flow is
not explicit (very bad in a complex app). It would be more clear to do
it this way:
@turbogears.expose()
def mypublicmethod(self, arg):
try:
someval = self.dosomething(arg)
except InconsistentStateError:
return cherrypy.HTTPRedirect(...)
return self.process(someval)
def dosomething(self, arg):
# oops, something isn't in the right state...
raise InconsistentStateError()
Again, I think it's important to keep application flow clear and
explicit.
return cherrypy.HTTPRedirect.
It looks like two incompatible meaning in one. Do you want to return or
redirect ? return to me means back to caller and in this case, it could
very well be decorated. So do I know if some logic is there ? raise
exception is cleaner to me in this case.
The problem with returning signal(or any form of return, even just let
the function fall through to the end) is that if there is N decorators,
each of them must code into this situation:
if redirect: return #let whoever knows how to do it do it, not me
else: do the proper decorator stuff
I would say that return means it is a "normal" flow that the page is
properly handled by the current function, may be it stucks different
value into the returned dictionary(including an alternative template)
but still normal. Redirect is "I don't know how to handle this and
returning to my caller is meaningless, another URL is better for what
follows".
Of course it is still a matter of choice(and convention) as I can
simply return simply string which in general would bypass all
turbogears handling.
I just want to point out the possible scenario one may be facing.
Can you give me a use case for that?
--
The root of the problem is the expected behavior of the function that
may cause a redirect.
>From Kevin's original example, the "dosomething" method returns an
object to be processed before the view is rendered. However, in the
case of an inconsistent state, it hijacks the control flow and does an
effective "GOTO someOtherPage". The problem here is subtle. In one case
we expect the function to return a piece of data that does not directly
affect the control flow (i.e. the return value does not contain a view
choice). In the other case (inconsistent state) the function plays a
direct role in handling the control flow, superceding any other
"normal" control flow handling.
I would argue that this is bad logic. If a function is not normally
aware of control flow it should NEVER directly cause a redirect. It may
raise an exception in case of inconsistent state, in which case the
parent controller (that knows about views) will cause a redirect if
needed. The parent controller must always have the final say in what
view is rendered, although it may delegate the view choice to a child
controller. In that case, we expect a single type of return value from
the child--an object that includes a view choice. The parent controller
has the final say because it can inspect the child return value and
keep or change it as needed.
I hope this is clear. I'm not trying to be a prick. In the end, I would
be happy to let this issue rest if there was simply a way to redirect
by passing a signal through the return path.
f(g(h(i(your_function))))
Which is the controller ? all f, g, h, i need to know your signal and
short circuit it back to the caller. Of course, you can say that is
more flexible as each can do different things because of the signal(and
in some case needed).
If all are written by the same person/same group of people, that may be
fine. But what if you use someone's stuff (say h) which knows nothing
about your exception situation(and the return value). It may boom.
One thing I do notice(and mentioned repeatedly) that many python coders
like exceptions so even you don't raise it, one of f/g/h/i may do so
because your return value may not be something they want. As simple as
dict[key] can boom when the key is not there.
So the end result is still, it depends.
The problem with GOTO is that it makes the logic hard to trace. That's
a bad thing when the application starts to get complex.
> Of course you can return back
> through the stack but what it actually like is :
>
> f(g(h(i(your_function))))
What's wrong with that? It's very predictable, which is what we want. I
would also like to point out that that's an unusually deep stack of
controllers to be handling the same request. In most situations there
would be one or two layers of controller nesting, not five. The deeper
calls (into model objects, etc.) know nothing about the web framework
and therefore cannot possibly raise an HTTPRedirect.
It would be nice to have a concrete use case for a situation where a
function five levels beyond the initial controller is raising an
HTTPRedirect.
> Which is the controller ?
If they can all cause a redirect, then they're all technically
controllers since they all have the ability to directly affect the
control flow (as I belive you are suggesting that any one of them may
raise a redirect exception). Normally as you get deeper in the call
stack (toward "your_function") the objects know very little or nothing
about the web tier, and therefore cannot possibly raise HTTPRedirect.
> all f, g, h, i need to know your signal and
> short circuit it back to the caller.
If they can all raise an HTTPRedirect then they all know about
TurboGears (or at least CherryPy) and it follows that they will all be
aware of the signal used to denote that the request should be
redirected.
> If all are written by the same person/same group of people, that may be
> fine. But what if you use someone's stuff (say h) which knows nothing
> about your exception situation(and the return value). It may boom.
>
> One thing I do notice(and mentioned repeatedly) that many python coders
> like exceptions so even you don't raise it, one of f/g/h/i may do so
> because your return value may not be something they want. As simple as
> dict[key] can boom when the key is not there.
I'm sorry, I don't understand what you're trying to say here. Can you
show me some code?
My question still stands...
def controller(x):
if x.redirect: raise HTTPRedirect
def encoding(x):
x.content = unicode(x.content)
return x
def your_func():
if dblookup:
x.content = lookup_content
else x.redirect = true
return x
controller(encoding(your_func())
Now if you don't find the value you want and just return a signal,
encoding would boom.
And in this case, the logic of GOTO is easier to trace. It just means,
the next stage is the HTTPRedirect. I view Web app as a pipe, not
conventional function calls from main style.
Unusually deep ? My page already needs that. turbogear.expose, some
login gator, some encoding stuff and other things.
But as I said, this is still a style, not that one is better than
another. I just highlight what one may face when certain style is
adopted.
None here. Sounds good.
Nice review. I've committed a patch to tg's svn that adds a allow_json
option so now you bad part should be solved :)
I was looking for this kind of redirect.
I'm just about to commit a patch to change so that format="json" sets
allow_json to True.
"Advertising directed at children is inherently deceptive and exploits children under eight years of age."
-- American Academy of Pediatrics
What exactly is InternalRedirect ? Does it mean I don't direct it to
the client but short circuit the flow back to cherrypy as if the client
want to access the redirect url(sort of a loopback and let cherrypy
redo the traverse to find the next handler) ?
Yes. That's exactly what it is.
I was looking for this kind of redirect.
"Computers are like Old Testament gods; lots of rules and no mercy."
-- Joseph Campbell