Code review comment for lp:~widelands-dev/widelands-website/fix_REMOTE_ADDR

Revision history for this message
kaputtnik (franku) wrote :

Thanks for pointing me to dict.get().... i am too stupid :-S

Regarding the tracking app: It's just my personal view... i don't like shipping an app with the widelands code when much code of it is potentially unused. 'Potentially' because i didn't took a closer look into it, just made it work for Django 1.8.

I like your suggestion for the code change. But i am unsure about returning 'None' if both entries didn't match (the previous code didn't caught this circumstance also). The question i am raising is: Since this function is used now for every request on a users ip, shouldn't it raise an error, or at least a 'fake' ip if both entries didn't match?

I have searched the code and some models containing a 'GenericIPAddressField' haven't set the field option 'blank=True', which means 'could be empty'. I found it only in wlimages and threadedcomments. So all fields without this setting expects a valid ip. This is the reason why e.g. the admin page for pybb Posts doesn't allow to save a change when the ip field isn't set.

Opinions?

« Back to merge proposal