Hey Brock,
Thanks for the review,
On 12/ 9/11 11:56 AM, Brock Pytlik wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/pkglint_mediated/pkglint_mediated-webrev
pkglint_action.py:
line 373: I'm still working to wrap my head around all this, but the
return on line 373 concerns me. Is it safe to return because, if an
action has a path, it won't also have a 'pkg' attribute?
That should really be a 'continue' rather than a 'return' - this section
of code previously just dealt with actions with 'path' attributes,
rather than either 'path' or 'pkg' attributes.
line 588: I'm confused as to how p couldn't be in self.ref_paths.
If I'm following correctly, ref_paths is a dictionary that's merged from
information from the linted packages and the reference packages. Since
(I assume) we only check the actions from linted packages, if the path
of that action isn't in ref_paths, hasn't something gone horribly wrong?
(Or does the comment on line 499 also apply here, in which case
duplicating the comment would be nice :) )
Yeah, you're right. That was a result of some long-dead code (removed
by e82acfe2f46e) previously when we just had coarse-grained support for
the "pkg.linted" attribute, we intentionally never added linted actions
to the various dictionaries so they'd never be seen by checks that used
those dictionaries. As a result, we could end up linting an action that
never appeared in those dictionaries. I've removed that here, and
elsewhere now.
line 639, 654: I think this if could be removed? Given an empty list,
variant_conflicts would just return an empty list, right?
Yep, thanks.
line 642: I'm surprised this is >1 instead of >0, especially given the
comment on line 622 which talks about at least 1. If nothing else, a
comment here is probably useful since it seems strange that one variant
conflict is ok, but 2 isn't.
You're right - I've changed that and verified it works.
line 686: nit, the 'list' inside 'sorted' isn't needed afaik
Yep, that's true, thanks. Fixed here and elsewhere.
Maybe this is already there and I just missed it, but is it worth
checking to see if a mediator attribute has been applied to an action
other than a link/hardlink action? Maybe it's not important since
attaching it to a file action once won't cause harm, and actually trying
to use it on file actions would cause other lint messages to fire.
It's not there at present, and is harmless for non-link actions. We
could add this check alright, though I wonder how likely it is people
are going to do that? For now, I'll leave the code as is.
setup.py:
line 737-738: if cddl_re.search doesn't match, then why are we doing a
sub? Could that ever do any substitutions?
Oops, of course.
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
