On Thu, Dec 20, 2007 at 03:19:30PM -0800, [EMAIL PROTECTED] wrote:
> http://cr.opensolaris.org/~johansen/pkg-unknown/index.html
This seems overengineered to me, given our current requirements, which are
pretty minimal. (I have the feeling that you weren't quite sure what to do
here.) I was thinking more along the lines of putting checks at 552 and
606 to make sure that cv was "in 'CV'", and perhaps ensure that pkg ==
"pkg". I'm not sure I understand the point behind the transformations you
allude to; perhaps an example would help? But if we're not doing anything
like that now, why complicate the code with all this stuff that basically
won't get run?
Still, there are some comments on the implementation choices that I believe
are worth making.
catalog.py:
- line 37 - no need for this; "set" is a built-in type as of 2.4
- line 742 - just keep this as a string -- "CSV" -- as that's already
immutable, and trivially becomes a set with "set(known_prefixes)".
- line 228 - "if new:" ought to be sufficient here.
- line 229ff - other than the calls to added_prefix(), all this is doing
is setting the "prefix" attribute to "known_prefixes".
- wouldn't we want to check if any of the prefixes in the catalog aren't
recognized (i.e., "if pfx_set - known_prefixes:")?
- line 546ff - so I would have just done this first test after the
split() on line 551 -- "if cv not in known_prefixes:". But the check
should actually be more specific here, because this bit of the code can
only handle "C" and "V" types. In fact, adding just that check was
pretty much the intent of this bug.
- Similar comment for the hunk at line 600, though here you don't do a
check inside the try clause, but you do in the except, which is
confusing.
updatelog.py:
- it occurs to me that we don't have any documentation of what the format
of an update log looks like, the way that Catalog's docstring does.
- It's probably worth putting a XXX comment in here to at least log a
warning when we retrieve an update log that contains an entry we don't
understand.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
