Code review comment for lp:~acoconut/systers/flexible_essays
Revision history for this message
| Robin J (robin-jeffries) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
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.
>
>
> > Why is replacements[
> > 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.
>
> >
> >
> > 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
>
> >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.
>