Code review comment for lp:~lukasz-czyzykowski/ubuntu-webcatalog/upgrade-to-django-1.5

Revision history for this message
Michael Nelson (michael.nelson) wrote :

12:54 <noodles> Hey there. Just looking at https://code.launchpad.net/~lukasz-czyzykowski/ubuntu-webcatalog/upgrade-to-django-1.5/+merge/158009
12:54 <noodles> Did you have that same piston surgery in the sca branch? I think I saw it and had a thought, but forgot to comment...
12:55 <noodles> Why wouldn't we fix that it piston instead? (I mean, update piston to use json rather than simplejson). Rather than doing patch-work on all of our apps?
12:58 <lukasz> noodles: I did that for all apps
12:58 <lukasz> noodles: patching seemed a quicker way to the end result than fixing piston
12:58 <lukasz> noodles: but I would really like to fix that dead project
13:01 <noodles> Yeah, it sounds quicker initially, but looks like more work in the long run.
13:01 <noodles> Also, what's the private attribute setting for 'HttpResponse._is_string = True' around line 152-54
13:02 <lukasz> another piston backward compatibility
13:03 <noodles> Hrm... do you mean, we could include the fix in an updated piston also?
13:03 <lukasz> it expects this attribute on the response object, and we never return response with iterator content, so that's why it's hardcoded
13:03 <lukasz> noodles: yup
13:03 <noodles> Do you want me to update piston?
13:04 <lukasz> noodles: I can do it, patches for all of the problems are on bitbucket
13:04 <noodles> Ah, sweet.
13:04 <noodles> Ah of course...
13:04 <lukasz> where do we store patched version?
13:04 <noodles> I was thinking we were talking about piston-mini-client (which is easy for us to update)....
13:04 <lukasz> no, that's django-piston
13:04 <noodles> Yeah,
13:05 <lukasz> which needs to be fixed
13:05 <lukasz> https://bitbucket.org/jespern/django-piston/pull-requests
13:05 <noodles> In which case, +1 for your work-arounds, just include a comment on that previous one.
13:05 <noodles> l152-154
13:05 <lukasz> HttpResponse?
13:05 <lukasz> ok

review: Approve

« Back to merge proposal