Code review comment for lp:~acoconut/systers/flexible_essays

On Thu, Aug 9, 2012 at 6:08 AM, Robin J <email address hidden> wrote:

> Review: Needs Fixing
>
> for this line
> if (question[2] != 'checkbox'):
>
> This implies that you want to do this for every kind of widget except
> checkboxes. No, you are doing it for every kind of widget you currently
> have except checkboxes. Make it clearer for the next person and have it say
> if (question[2] == 'small text' or question[2] == 'large text'):
>
> you also want to rethink your structure a bit more -- you have two tests
> for not being a checkbox, and you should be able to combine them into 1.
> (again, my guess is that if we add new widgets that are not texty, you may
> want to treat them differently, so explicitly list the question types you
> are using.
>
> If these lines aren't useful to you, delete them; it's no longer accurate
> since you changed the utf-8
> command = "self.store.find(Subscriber,Subscriber.mailman_key ==
> unicode(key,'utf-8')).set(deleted = False)"
> if DEBUG_MODE:
> syslog('info', 'DlistUtils(subscribeToList):Executing
> query:\n%s', command)
>
> You defined acceptAnswer, but earlier you have almost identical code, why?
> Why not call acceptAnswer there?
>
> And why aren't these identical? why do you have
> unicode_key = unicode(key, "utf-8")
> in one place and
> userID = addr.decode('utf-8')
> in the other
>

Because I had to figure out how to decode strings and found the addr.decode
way and then I saw somebody else had done it using the unicode function.

I changed all the other things you suggested and added the ordering by
subscription time idea to a tasks list so that I remember to do it later.

I'm pushing the new code now.

> --
> https://code.launchpad.net/~acoconut/systers/flexible_essays/+merge/118537
> You are the owner of lp:~acoconut/systers/flexible_essays.
>