On Thu 24 Jul 2008 at 06:04PM, Brock Pytlik wrote:
> After talking with Brad, I've decided to post this webrev which includes
> his earlier comments.
> My tentative plan is to push it back to the gate in roughly 24 hours
> unless I hear at least a "wait, I need more time to code review." If
> that's the case, just let me know and I'll hold off for a bit.
>
> Thanks to everyone who helped improve this code recently, it's appreciated.
Couple of minor things-- I finally got to looking at the manifest
changes. None of these are so important that they should hold you up;
it's my bad to be so late to the party. I mostly looked at manifest.py
since you indicated to me the other day that it'd be good if I did.
manifest.py: 261: is this comment out of place? Seems like it should be moved
down to the "if isinstance(v, list)" code a few lines down. It'd be nice to
have a comment about the special casing you're doing for AttributeAction.
client.py:
1619 + error(
1620 + "\n\nThis is an internal error, please let the
\n" + \
1621 + "developers know about this problem by emailing
\n" + \
1622 + "[email protected] and including the
\n" + \
1623 + "above traceback.")
At a minimum, please format a little wider, closer to 80 columns. I'd really
like to rework this into something a little more, umm, worked out, soon. I
swear there is a related bugid, but I can't find it. I might rather direct
people to http://defect.opensolaris.org. For now, put it back this way,
though-- it'll prod us to fix this ASAP. Can we at least get a bug filed for
this specific change?
progress.py: index_get_progress(): needed? If you need it there, perhaps
raise a NotImplementedError?
version.py: if you have time, please update tests/perf/fmribench.py for the new
hash functions you've defined (just basically copy the fmri
hash code).
For Version objects, I'm not convinced that you need to hash on
anything more than the timestr (if it is present), but the code is
fine as is. To see an example of what I mean, check out __hash__
for fmris.
n.b. 3500 lines is a lot of delta! Nice work. Looking forward to having
this in b95.
-dp
--
Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
