On Mon, Sep 6, 2010 at 1:48 PM, Gary Poster <email address hidden> wrote:
> Review: Approve
> Very nice catch with the check for isinstance of BaseException.
>
> I question the view pattern for the registration and lookup--using
> "index.html" as the distinguishing marker. I'd prefer a custom interface
> myself. If, in retrospect, you agree, great. If not, I can see where you
> are coming from: leave as is.
I was following the pattern used for the other error views. I don't feel
strongly about it one way or the other so I'll change it.
> We need a linter.
Yep. I'll take a quick look to see if pyflakes or similar is easily
usable with our lazr buildouts.
> Either ``from types import ClassType`` in
> src/lazr/restful/error.py shouldn't be there now, or it should have been
> caught earlier. Which is it?
That's left over from a previous revision of the code. Removed.
> In error.py, you say "# Bug 11512 describes the possibility of controlling
> the HTTP status of...." I think you mean bug 631711.
On Mon, Sep 6, 2010 at 1:48 PM, Gary Poster <email address hidden> wrote:
> Review: Approve
> Very nice catch with the check for isinstance of BaseException.
>
> I question the view pattern for the registration and lookup--using
> "index.html" as the distinguishing marker. I'd prefer a custom interface
> myself. If, in retrospect, you agree, great. If not, I can see where you
> are coming from: leave as is.
I was following the pattern used for the other error views. I don't feel
strongly about it one way or the other so I'll change it.
> We need a linter.
Yep. I'll take a quick look to see if pyflakes or similar is easily
usable with our lazr buildouts.
> Either ``from types import ClassType`` in restful/ error.py shouldn't be there now, or it should have been
> src/lazr/
> caught earlier. Which is it?
That's left over from a previous revision of the code. Removed.
> In error.py, you say "# Bug 11512 describes the possibility of controlling
> the HTTP status of...." I think you mean bug 631711.
Indeed. Fixed.
--
Benji York