Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.97k stars 886 forks source link

An analogue of pyramid.view.view_config which registers an Error View. #1646

Closed zupo closed 8 years ago

zupo commented 9 years ago

Much like notfound_view_config and forbidden_view_config I believe we should have an error_view_config which is analogues be the same as registering a catch-all exception view.

I can start working on this, but I'd like to get a few +1's that it's something that could get merged.

mcdonc commented 9 years ago

How would it differ from:

   @view_config(context=SomeException)

Do you mean it would be registered for Exception unconditionally?

mmerickel commented 9 years ago

I guess he's saying it'd be the same thing.

I think this is not worthwhile right now because the notfound and forbidden views actually do some useful things in their api. For example append_slash. This seems like it would be just adding an api to make it more obvious to people that the feature exists.

mmerickel commented 9 years ago

This would need to be named exc_view_config or exception_view_config instead of "error" to make it clear its purpose as well. I think it's potentially interesting to have an exception_view_config that had an optional argument context= or exc= that defaulted to Exception but could be overridden with other exception types.

mmerickel commented 8 years ago

So far the only reasons I can think of for adding an exception_view_config(exc=Exception) is because 1) it is more obvious and 2) it may set a default permission of NO_PERMISSION_REQUIRED. However I'm not sure this is compelling enough!

Also I have the last 3 comments on this issue in the last 8 months so apparently no one else feels strongly about this feature. Closing the issue but feel free to comment and we can always reconsider.

mmerickel commented 8 years ago

I think I came up with a reason for this to exist. Currently pyramid has no good way to register a view as an exception-view only. All views are registered as both regular views and optionally as an exception view, but never just as an exception view.

As I've built out the view derivers there's certain derivers that I want to only affect exception views and currently the only way to do it is via a per-request check for request.exception. I can't do it at config-time to avoid affecting normal views.

This feature could be a thin wrapper around a new exception_only option to config.add_view(). This would also be used by notfound and forbidden views to their benefit.

There's some subtleties in pyramid's approach I mentioned in the first paragraph though that may need to be worked out.

mmerickel commented 8 years ago

We would want an exc-only and regular+exc view registration to conflict on the same parameters for sure. The part that I'm struggling with is the effect of supporting an exc-only view and regular+exc view on the same MultiView instance. Currently the MultiView doesn't need to think about exc vs regular views, it just thinks about views. It would need to grow some knowledge that some of its views are exc-only and then check request.exception prior to considering them in its match algorithm. Alternately config.add_view would need to register 2 separate multiviews but I think that is a non-starter for many reasons.

mmerickel commented 8 years ago

@latteier has there been any progress on this since the pycon sprints? If you were able to get anything done but have lost steam it'd be good to push a branch which we can evaluate and work from.

latteier commented 8 years ago

@mmerickel thanks for the reminder! I'll push my branch, then we can look at it.

There are lots of changes that I have meant to do, that I haven't gotten around to yet. For example, I think that if we have a generic exception view registration then we can probably refactor the not found view and forbidden view stuff to use it.

latteier commented 8 years ago

@mmerickel I think that I don't have permissions to push a branch.

digitalresistor commented 8 years ago

@latteier Create a fork of the repo and push to your own fork, then submit a PR for the changes. That way any pushes you do from then on will automatically show up in that PR.

latteier commented 8 years ago

@bertjwregeer Would you prefer a PR from my fork rather than me pushing my branch? @mmerickel just gave me permissions to push my branch. I think that idea was that it would be easier for others to collaborate on the branch rather than on a PR.

digitalresistor commented 8 years ago

If you have access, by all means push a branch. We can create a PR from that, and then Travis and friends will pick it up and rebuild it as we collaborate :-).

mmerickel commented 8 years ago

Closed by #2660.