Code review comment for lp:~dylanmccall/harvest/gsoc-client-stuff

Revision history for this message
Daniel Holbach (dholbach) wrote :

Thanks a lot to your enormous, great work. This is BEAUTIFUL!

I'd love to have reviewed this in several branches, and for a short time considered breaking this up into several parts, but I don't want to claim credit for your work, so I refrained from doing that. :-)

The logical sections that I was able to make out:

--- Timer and version_name
--- harvest/common/{context_processors.py,middleware*,utils.py} and harvest/settings.py

harvest/version is written by harvest/opportunities/management/commands/release.py and the version name string would get overwritten.

--- Ordered Dict and Javascript stuff
--- harvest/common/ordereddict.py and harvest/media/js

Might be worth pointing out where you got that from. Maybe we should create ./EXTERNALS or something for that and note down URLs and licenses.

--- Filters changes
--- harvest/filters/

Only minor, but I personally prefer to avoid compound statements if possible. (PEP 8 is on my side on this too ;-))

copy import in filters.py seems unnecessary

--- Template and Views removal
Good work.

--- Login is broken
Traceback (most recent call last):

  File "/usr/lib/pymodules/python2.6/django/core/servers/basehttp.py", line 279, in run
    self.result = application(self.environ, self.start_response)

  File "/usr/lib/pymodules/python2.6/django/core/servers/basehttp.py", line 651, in __call__
    return self.application(environ, start_response)

  File "/usr/lib/pymodules/python2.6/django/core/handlers/wsgi.py", line 245, in __call__
    response = middleware_method(request, response)

  File "/usr/lib/pymodules/python2.6/django/middleware/locale.py", line 21, in process_response
    patch_vary_headers(response, ('Accept-Language',))

  File "/usr/lib/pymodules/python2.6/django/utils/cache.py", line 130, in patch_vary_headers
    if response.has_header('Vary'):

AttributeError: 'NoneType' object has no attribute 'has_header'

Thanks a lot for all your work on this. I'll have a closer look into the filters and javascript stuff later on.

« Back to merge proposal