Code review comment for lp://staging/~anandjeyahar/beeseek/Fedora12

Oh i had run link and make install.
Both went through, so i just committed. Will fix these and post again.
==============================================
Anand Jeyahar
http://sites.google.com/a/cbcs.ac.in/students/anand
==============================================
The man who is really serious,
with the urge to find out what truth is,
has no style at all. He lives only in what is.
                  ~Bruce Lee

Love is a trade with lousy accounting policies.
                 ~Aang Jie

On Mon, Aug 30, 2010 at 15:11, Andrea Corbellini <
<email address hidden>> wrote:

> Review: Needs Fixing
> First of all, thanks for your contribution.
>
> Your branch is good: the code you have changed works as it should and does
> it jobs. However there are few problems with it. The most important thing
> that you should do is fixing the tests: in fact, if you run "make check" or
> just "tools/test-node", you'll notice some failures introduced by your
> changes. You won't have to change nothing special: just 'USER-AGENT' ->
> 'HTTP_USER_AGENT' and so on, so it shouldn't be difficult to fix the tests,
> but please tell me if you have troubles.
>
> There are also a few minor problems with your code:
>
> * You placed the "name = 'HTTP_' + name.replace('-', '_')" inside the
> try..except block. If an error occurs while concatenating 'HTTP_' with
> `name`, than it means that there is a bug in our software that we should
> fix. The error shouldn't pass silently, but instead must raise an exception.
> It means that you should place the code out of the try..except block.
>
> * Could you please replace 'name.replace(...)' with
> 'name.upper().replace(...)' (removing the call to upper() in the line
> below)? This will make things a bit faster (although not significantly),
> since the prefix 'HTTP_' won't be touched by upper().
>
> * In webserver.py you removed the TODO and the `name = ...` line, and
> that's good. But with this change now the loop is totally useleess: you can
> use the update() method of the request_meta dictionary:
> request_meta.update(headers).
>
> This last three problems are actually just nitpicks, but I would like to
> see the tests fixed before approving the merge.
>
> Thanks,
> Andrea
> --
> https://code.launchpad.net/~anandjeyahar/beeseek/Fedora12/+merge/34012
> You are the owner of lp:~anandjeyahar/beeseek/Fedora12.
>