On Wed, Jun 11, 2008 at 01:13:46PM -0700, Brock Pytlik wrote:
> I didn't realize I was supposed to put evaluation back into the bug. Are
> there a couple of bugs that have a good example of this so that I can see
> what kind of information should be provided?
The idea is to explain why the observed behavior is occurring, what it
should be instead (if not mentioned in the description), and any
interesting analysis you had to do to gather that information.
In this case, mentioning what caused the file not to be found -- an
explanation of how the relative directories needed to line up just so --
would be what's called for. Doesn't have to be verbose or flowery unless
you're in the mood.
I can't find an exemplary bug at the moment. Usually K is pretty good
about this, but none of the bugs he's currently working on has any real
evaluation (I guess they're RFEs).
> The problem is that, when run from cli-complete, t_actions.py doesn't know
> where to look for the files. Relying on a relative directory relationship
> between cli-complete.py and t_actions.py seemed like a brittle approach to
> me as did relying on either being run from a particular directory.
Okay.
>> - The string + variable + string constructions in the manifests are,
>> IMHO, difficult to read. I'd use %s expandos instead.
>>
> that's funny cause I find the %s expandos very difficult to read (ie, I
> purposefully avoid using them when writing code), but if that's the
> standard style we're using as a group, then I'll switch
Hm, okay. I guess the structure stands out better when you're looking at
the real code and strings are highlighted differently. It doesn't come
through in the patch, though.
I don't care too much here; I'd be more bugged in the rest of the code.
So if no one else has any opinions, feel free to leave it alone.
I would, however, try to keep each logical line on a real line, even if
that means they go over 80 characters.
>> - You don't seem to do anything with "empty". Does that not cause
>> problems, too?
>>
> I'm not sure what you mean. The empty file was in fact, empty. It gets
> created in the set up that loops over misc_files and has no data in it.
I missed it, sorry.
>> - Ditch the ident string in the ftpusers file.
>>
> Ok, it was in the original file, so I wanted to duplicate them exactly (I
> wasn't sure exactly which lines were critical for the test).
Yup. I thought about asking you to ditch the comments entirely, but it's
probably good to keep a couple lines of it in.
>> - line 139ff: Not a requirement, but I'd probably use eval here to
>> reduce the elif cluster to a single line.
>>
> Can you explain what you mean by using eval? I don't see what you mean.
for f in misc_files:
...
try:
file_handle.write(eval("self.%s_data" % f))
finally:
...
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
