On Mon, 2010-08-16 at 15:47 -0700, Brock Pytlik wrote:
> On 08/16/10 03:59 AM, Tim Foster wrote:
> > Checker subclasses implement the checks themselves, with each object
> > building a list of those implementing methods. The engine instantiates
> > Checker objects, and iterates over their list of checker methods,
> > calling each for every manifest and action presented for linting.
> >
> > I'm expecting all pkglint plugins to be subclassing ManifestChecker,
> > ActionChecker or (eventually) ContentChecker, implementing their own
> > checker methods.
> >
> >
> I think I'm following this. Since a checker class can have multiple
> checker methods, when is it appropriate to add a checker method to an
> existing class versus creating a new checker class for the method?
Aside from the different subclass types (manifest vs. actions) check
methods that may want to share data-structures with each other or have
similar functionality should be in the same class, using a common
setup() method to initialize those - for example,
pkglint_action.PkgDupActionChecker.
If we're adding checks that are generic across all pkg(5) users, they'll
belong in pkg_manifest.py or pkg_action.py - which exact class they go
in doesn't matter a whole lot, unless they're sharing data-structures.
In general, I'm hoping that the plugin mechanism for pkglint feels
familiar to anyone who's ever turned on a mercurial extension in
~/.hgrc, and that the writing of pkglint checks, and the runtime
mechanism of finding and discovering those feels just like writing tests
for a generic unit test framework.
> >> So we only show the tracker in DEBUG level of verbosity? I think I
> >> would have expected the tracker to be shown by default.
> >>
> > I did experiment with that, but found that when not redirecting stderr
> > to siphon off lint messages, the output looked pretty messy as the lint
> > errors would co-mingle with the progress tracker. I could try to fix
> > that if you think the tracker would be better - certainly on very large
> > lint runs (3,000+ packages) that spend a long time in engine.setup(..)
> > the tracker is nice to have.
> >
> Hmm, I had missed that the tracker would interleave w/ the lint
> messages. Perhaps a switch to turn the tracker on then? I'm working from
> memory now but I thought that going to debug level increased the amount
> of output in general. It might be nice for a user to be able to turn the
> tracker on (or off) independently from the logging level?
> I don't have strong feelings about this though so if what I'm saying
> doesn't make sense or is a pain to implement, don't worry about it.
Yep, I'm messing about with this at the moment to see if there's
anything I can do. I have it printing blank lines before messages if it
detects we've forced the tracker to be on, the problem is, it needs to
query tracker state to see if the tracker has printed an update where we
get, eg. 1 error per package, and the tracker has skipped a few updates.
> > I've re-numbered the checks in that class so that they're contiguous
> > (better to get this right now - it would be wrong to re-number after it
> > gets putback)
> >
> >
> Are the id's eventually expected to be an interface with some reasonable
> level of stability? I hadn't realized that the ids were more or less
> assigned for all time.
Yes, I think it'd be polite for people who frequently run pkglint to
always have the same lint name+id pairs to map to the same test across
different versions of pkg, so that they can see at a glance what
problems are occurring.
> >> 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.
> >>
> > This is all about determining when to overwrite dictionary entries
> > generated from a reference repository with what's being presented for
> > linting. (and thus, we presume, potentially published to that reference
> > repository)
> >
> > In particular, we're ignoring the timestamp field if present, since
> > that'll get updated once we publish the package - so we're biasing the
> > comparison to treat packages from the lint repository as being newer if
> > the only thing different between a lint package and a reference package
> > is the timestamp.
> >
> >
> Hmm. Ok, I get the idea of preferring packages from the lint repo over
> packages from the reference repo. What I'm having trouble connecting is
> why a different decision is made with regard to which is newer depending
> on whether both packages have a version @.1 or @.0. What about .2
> through .9? Is it the second position that matters or the last one?
I think you're misreading the example (which I should probably reword)
./src/modules/lint/pkglint_manifest.py
We use a slightly different
279 algorithm than pkg.fmri.is_successor to do
280 this, largely because manifests for linting
281 may not have timestamps attached yet. If
282 they do have timestamps, we ignore them.
So:
283
284 [email protected] is newer than [email protected]
285 [email protected] is newer than
[email protected]:20100809T225106Z
286 [email protected]:20100809T225106Z is newer than
[email protected]
287 """
I really just needed to say that timestamps were ignored for packages of
equal versions, favoring packages from the lint repository, otherwise
standard version comparison applies.
> It's possible that I'm just not groking the example, but how I interpret
> it is "When the package version ends in .0, we use one set of rules
> where having a time stamp means you're newer than not having a time
> stamp. When the package version ends in .1, we use a different set of
> rules where having a timestamp means you're older than not having a time
> stamp."
> Also, in your example, I'm not seeing any mention of lint versus
> reference repo, so maybe that's the step I'm missing?
in the "pkg@" examples, I'm comparing the lint against the reference
package. I'll reword the example.
[snip]
> My concern is that we're logging messages, rather than collecting
> objects and later converting the objects to strings. As an analogy, this
> has a similar feel to raising strings as exceptions instead of having a
> class hierarchy for exceptions.
Aha, I understand. While we could easily collect the raw LintMessage
objects, you'd still need to read the LintMessage.msgid (which is
separate from the LintMessage.msg, the body of the lint message) but the
fact they're both strings is what you're concerned about.
> For example, if I wanted to make graphical interface which colored
> packages according to the types of lint errors they contained (Content
> vs Action vs Manifest), I don't think there'd a way to do it short of
> parsing the error messages and maintaining a mapping from id -> type
> some place.
>
> I don't see us doing that anytime soon, which is why I mentioned a low
> priority RFE.
Sure - I'll have a think about it a bit more, and file an RFE.
I'm going to wait a few more days for some more feedback, while
experimenting with the tracker output and see if I can improve that.
thanks again for the feedback,
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
