Code review comment for lp:~dylanmccall/harvest/harvest-dylan-m

Wow!

Thanks Dylan.

31 + url_params = list()

seems to be superfluous.

Would there ever be a desire for

36 +def current_url_with_parameters(request, new_params_dict):

to remove parameters? That can be something that is added later if needed
though.

46 \ No newline at end of file
453 \ No newline at end of file
673 \ No newline at end of file

Fixing those would be nice.

149 + #note that this currently stores parameters that aren't relevant to the FilterSystem
150 + #we need to do that so we don't eat them in get_url_with_parameters
151 + self.set_parameters(request.GET)

is that still needed with the code to get the original params in get_url_with_parameters?

Where you mark a method as abstract and don't implement in then consider

    raise NotImplementedError(self.<abstract_method>)

which will give an error if the subclass doesn't implement it, instead of
silently doing nothing.

244 + @param choices_dict: Optional value to be used instead of internal value

that's not a parameter to that method, same in the next method.

Where you use mark_safe you should add a comment stating why that string is
known to be safe.

Do the default parameters make sense? I'm not sure we should be defaulting to
"ged" at least.

Overall this is some seriously great code, nice work!

Thanks,

James