Code review comment for lp:~acoconut/systers/flexible_essays
Revision history for this message
| Ana Cutillas (acoconut) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
On Sat, Jul 28, 2012 at 6:13 AM, Robin J <email address hidden> wrote:
> On Fri, Jul 27, 2012 at 10:43 AM, Ana Cutillas <email address hidden>
> wrote:
>
> > On Fri, Jul 27, 2012 at 6:47 AM, Robin J <email address hidden> wrote:
> >
> > > Review: Needs Fixing
> > >
> > > I would move the comment "added to support subscribe essays" up to the
> if
> > > statement, as right now it looks like just that one line was added for
> > the
> > > essays.
> > >
> >
> > OK, done.
> >
> > >
> > > I would also suggest that you mark lines that are just for your
> debugging
> > > with a comment like "debugging" so that a) I can tell that's what they
> > are,
> > > and b) you can find them to remove them later. I assume lines 53,
> 55-57
> > > are debugging statements?
> > >
> >
> > Line 53 is not mine and it looks to me that when somebody is logging
> > through syslog for debugging, they write to the info file, in this line
> > they're writing to the error log, so maybe they wanted to keep a log of
> > this error every time it happens? I can add the if DEBUG_MODE statement
> if
> > you think it shouldn't be always stored. Let me know.
> >
> > 55-57 was junk, I got rid of it.
> >
>
> Apparently we are not talking about the same line numbers. I mean the ones
> in the diff file.
> index = 0
>
> and
>
> box_name = "essay.
> syslog('info', "Box name: %s", box_name)
> index = index + 1
>
> but it looks like this is gone now.
>
> Yes, I cleaned that up.
> >
> >
> > > Why is replacements[
> get
> > > rid of it?
> > >
> >
> > Yes, we're not using the replacements[
> > mean some cleaning up is due in all the template related files. I'll
> leave
> > this for the end when I do the cleaning up.
> >
>
> OK, I would suggest you put in a comment with at TODO to remind yourself.
>
>
> >
> >
> > > In subscribe.py you should make sure that the essay variable that is
> > > passed in is the same type (a list?) whether there is just one question
> > or
> > > many (I can't see where you construct that data structure -- where does
> > > that happen?)
> > >
> > > (in subscribe.py) It seems pretty dangerous to have the question label
> > > just be the order in which you process the questions. Python does not
> > > guarantee that in a loop that says
> > >
> > > for foo in foo_list:
> > >
> > > that you will process each foo in the same order every time. Also,
> what
> > > happens if I reorder these questions (you don't have that functionality
> > > yet, but you need it)? Do you want them to have different question
> > > numbers? I think you want the question name (like 'talk') here.
> > >
> >
> > Mmm... are you suggesting using the question name as the question ID?
> That
> > would work.
> >
>
> Yep, as you understood.
>
> >
> >
> > >
> > > You also seem to have hard coded the version to 1. Why is that?
> >
> >
> > > In DlistUtils.py, the FormQuestion class, why are you putting together
> > > that command statement, that seems just to put something in the info
> log,
> > > when your next statement does it more directly? That confused me for
> > quite
> > > a while.
> > >
> >
> > I know, it confused me too, but it's what every other class does! So I
> > thought it was something you guys had agreed on?
> >
>
> Nope, I think that when Malveeka did the STORM code, this was just how she
> set up her debug statements. We weren't doing code reviews that summer...
>
> Feel free to ask when things are not clear.
>
> >
> >
> > > If form_question_id is autoupdating, then what are you doing in line
> 144?
> > >
> >
> > Line 144 is not mine or close to mine, I wrote around the 300 lines.
> >
>
> I'm not sure what was going on with the line numbering. This is the line I
> meant
>
> formQuestionID =
> self.store.
>
>
I am saving in formQuestionID the return value from storing which is the
number that has been autoupdated. Does that make sense?
>
> >
> > >
> > >
> > > In DlistUtils.py, where did you add remove_dlist() I don't see it.
> > >
> >
> > I haven't done anything about removing dlists. Had we talk about
> something
> > like that?
> >
>
> I was refering to the line in rmlist
> DlistUtils.
> (which, taken with the lines above it, seems to be that you encapsulated
> those lines into a method, but I didn't see it in the changes to
> DlistUtils.py
>
>
> I haven't touched rmlist at all. So I don't know how that changed :-/
> >
> > >From your other email:
> >
> > 2. version ought to be stored with the question. You would update the
> > > version whenever someone edits the question rather than replaces it (I
> > > would think an edit would be one of several types
> > > -wording change
> > > - answer type
> > >
> > > So I would start when someone creates a question as version 1, and then
> > > when it is edited, that gets incremented. So it's part of the
> definition
> > > of the question (like the question text), and you don't need to store
> it
> > in
> > > the pck file (every question could be a different version level.).
> >
> >
> > Alright that's pretty easy.
> >
> >
> > > I think the big reason you don't understand version yet is that you
> > > haven't figured out what you are going to do with it. If you were to
> > > sketch out what the UI is going to look like for the administrator,
> when
> > > they go to approve these folks, and then think about what you want it
> to
> > > look like if you have two people with a) different versions of the same
> > > question, b) different questions, that might help you figure out where
> > you
> > > want to keep the version number.
> >
> >
> >
> > > --
> > >
> >
> https:/
> > > You are the owner of lp:~acoconut/systers/flexible_essays.
> > >
> >
> > --
> >
> https:/
> > You are reviewing the proposed merge of
> > lp:~acoconut/systers/flexible_essays into lp:systers.
> >
>
> --
> https:/
> You are the owner of lp:~acoconut/systers/flexible_essays.
>