On 12/07/11 23:58, Tim Foster wrote:
hi all,
I've got a code review here that brings mediated link support to
pkglint, checks for matching attributes in reference-counted legacy
actions, and allows variant.debug variants.
I've also included a change to setup.py that does a better (but
uglier) job when CDDL stripping, replacing the CDDL text with blank
comment lines so that Python line numbers match between stripped and
non-stripped files.
Webrev at:
https://cr.opensolaris.org/action/browse/pkg/timf/pkglint_mediated/pkglint_mediated-webrev
Comments would be most welcome,
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
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? In a sense, at
most one of the two passes through the loop at line 297 will do any real
work. If that's right, it's probably worth a comment or two about that
at both line 297 and line 373.
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 :) )
line 639, 654: I think this if could be removed? Given an empty list,
variant_conflicts would just return an empty list, right?
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.
line 686: nit, the 'list' inside 'sorted' isn't needed afaik
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.
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?
Otherwise lgtm,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
