On 08/12/10 07:45 PM, Tim Foster wrote:
Hi all,
I've got another pkglint webrev here that I'd really appreciate code
review comments on please?
http://cr.opensolaris.org/~timf/pkglint-webrev-2
General comment:
Indentation on docstrings seemed inconsistent w/ the other files we have
in most of your new files.
doc/pkglint-plan.txt:
nit line 100: can you accept multiple repositiories or only one? ("a
repositories" is what's there now)
248, 252: signature actions should be allowed as part of renamed and
obsolete packages. Since this may not work outside of pkglint after my
putback, I'm fine with this being an rfe.
man/pkglint.1.txt:
lines 11-13: This don't seem to be following the indenting conventions
of our other man pages.
42-43: what happens if I combine cache and -l and -r?
50: Is "/* LINTED */" a convention known outside ON/Oracle? This
paragraph would have made more sense to me if the first sentence was
something like:
pkglint(1) allows package authors to bypass checks for a given manifest
or action.
79+: our other man pages don't look like this but I've heard we're maybe
moving in this direction? Either way, probably worth getting clarification.
lint/base.py:
I couldn't quite follow the relationship between pkglint_id and name.
I think some explanation of what an engine is, and a general idea of how
the pieces fit together here would be useful. For example, how do I know
if I'm defining a new type of check or creating a new instance of an
existing type? Why is the base Checker doing a check for conflicting
variants?
It's probably worth including a note that if a new type of check is
added here, the code in lint/engine.py will need to be changed so that
the new type can be used.
40-59, 131-132, 142-144, 103-104: indentation looks wrong
64: Can there only ever be one instance of a Checker object in existence
at a time? I'm not seeing why the ConfigParser object should be shared
among them.
93: what's the type of item?
109, 115: It seems like if 109 enumerated the items as they were
iterated over, then 115 could start at i+1 instead of looping over the
entire list since, as you mention, intersection is commutative. I'm also
picking up my separation of the VariantSets class into two separate
classes with different functions. I'd like to talk with you offline
about what I'm looking at doing to make sure that what you need will
still be provided with the changes I have in mind.
util/publish/pkglint.py:
I would have expected this to be in src/, or at least in the same
location as pkgsend/recv/sign etc... Do we have clear guidance on when
things belong in util/* and when they don't?
lint/engine:
51, 61-138, 182-184, 196-197, ...: indentation looks off
79-85: I don't remember seeing explanations of what a reference or lint
image are by this point.
104-107: This doesn't really seem to be a sentence, and I'm having
trouble unpacking the clauses.
If I understood correctly, here's a suggested rewrite:
Determines whether to perform a subset of checks which may only make
sense for published packages on manifests passed from the command line.
214: could "pkglintext" have some separators added to it? "pkg_lint_ext"
"pkg.lint.ext"? If it's too late for that change, no problem.
255-280 (and other places in other files too I think): This is just a
personal pet peeve, but could we choose another format for this? If we
ever added another argument to the function with a name longer than
"lint_manifests" then we have to go back and reindent all of this text,
which is both a silly annoyance and also clutters up the diff.
354: extra line
377: shouldn't there be an error/exception/warning raised if there's a
checker which isn't a subclass of the 3 expected classes?
401: Is there a reason to make passing None as the value of api_inst
doesn't cause an error instead of returning immediately?
445: Should this be > instead of <? Looking into log.py, I guess not.
Intuitively, I would have expected more debugging to be represented by
bigger numbers, but it's not a huge deal to me either way. So we only
show the tracker in DEBUG level of verbosity? I think I would have
expected the tracker to be shown by default.
477-479: weird capitalization here
I'm confused by the comment on lines 533-534 about only supporting one
publisher per image, and the code on lines 561-586 which seems to be
adding multiple publishers to the image.
593, 613, 633: So having pkg.linted=False or pkg.linted=True would both
cause the action/manifest/content to not be checked?
Also, is the check on line 633 redundent with the one on 613 or do you
anticipate being able to check content without checking an action in the
future?
625: I don't quite follow the XXX comment.
lint/opensolaris.py:
51: Probably just my own lack of knowledge here but I thought we were ok
with usernames longer than 8 chars.
66: I think the ":" check is redundant given the re that follows (ie, no
username with : in it could match that re).
99: why not just base.ManifestChecker.__init__(self, config)? (same
comment for the other checker modules)
106: why is this XXX'd?
lint/pkglint_action:
152-153: I don't understand this comment, mostly the notion of search
pattern here has me lost.
181: A brief comment here about why we want to merge the lint and ref
dictionaries now but it was ok to have the lint_manifests and the
manifests from engine.gen_manifests(engine.linst_api_inst ... ) put
into the same dictionary from the start would be helpful.
203: Was the check with id 002 removed?
273: The XXX should either be removed from the comment or the code on
line 279 should be removed?
310, 374: The manifest parameter doesn't seem to be used?
538: I'm having trouble understanding what this is trying to check. Is
"us" pkg5?
540: missing comma or period between us and check?
lint/pkglint_manifest:
234-237: wrong indentation
286-287: I don't understand why @ .0 having a timestamp means it's newer
than one without a timestamp while @ .1. it means it's older than one
without a timestamp.
util/pkglintrc:
1: I'm not sure what that tag means or when I would (or would not) put
it in my own pkglintrc file.
util/publish/pkglint:
having an option to direct the output to a file would be nice
I'm a little concerned about how the logging interface is being used
here. Mostly, it seems like if someone ever wanted to build an alternate
interface on top of this code, it would mean having to parse the strings
in the logger output. I realize it's probably late in the game to switch
that out, but would you consider filing a low priority RFE about it?
60-63: same comment about indentation as in the man page/
66-83: Could you comment on this approach vs the one we've used
elsewhere? Would you suggest we should look to move all our code to this
approach rather than our current one?
228: Why codecs.open? Is this something we should be doing in other
places across the code?
This looks like a good approach, I'm looking forward to having it in the
gate and getting our builds lint clean. Thanks,
Brock
[snip]
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
