On 12/08/11 18:41, Tim Foster wrote:
Hey Brock,
On 12/ 6/11 01:37 PM, Brock Pytlik wrote:
https://cr.opensolaris.org/action/browse/pkg/bpytlik/19009-v3
Just taking a look at this - this was pretty tough to review,
dependencies.py is one of the more complex parts of our code I feel -
perhaps I'm just not familiar enough with it yet.
No disagreement there.
I wonder if this code is sufficiently complex that it warrants a more
general discussion of the approach to be put in
./doc/pkgdepend-primer.txt or somesuch?
I'm happy to write such a doc, but I'm not sure what I'd put in there. I
think the basics of pkgdep are pretty simple, really things only get
confusing when variants get involved (at least from my perspective).
Maybe we can talk next week about the kind of info that might get put in
it. I don't think it should get as low level as documenting code or
functions, since that info should be comments in the code itself.
In the meantime here's some comments:
src/modules/publish/dependencies.py
line 846, 950 - array indices are hard to read here, could we use a
named tuple here instead?
For 846, I believe it's possible to use a named tuple. I'll look into it.
I think for 950, it's really not because the things in lst could contain
different information on different calls to
group_by_variant_combinations. The only thing required for that function
to work is that the last thing in the tuple be a VariantCombination.
line 852, 1303 - typo 'dependency'
Thanks, for some reason I make that typo I bunch. I thought I'd caught
them all.
line 870 - similarly, array indices are hard to deal with
l[0][2]?(l[0][2] corresponds to t[4] from line 846, right?)
Yep. The l[0] I can't do anything about, that's the first item of a
list. I'll see what named tuples offer here as well.
line 950 - the comment here is terse - could it explain a bit more
what it's trying to do when grouping? I was a bit lost in here, I admit.
I'll give it another shot. How's this:
"""The goal of this function is to produce the smallest list of (info
list, VariantCombinations) tuples which has the following properties:
1. The intersection between any two satisfied sets of the
VariantCombinations must be empty.
2. If a piece of information was satisfied under a particular
combination of variants, that information must be paired with the
VariantCombination which has that combination in its satisfied set.
Note: A piece of information can appear more than once in the result.
The 'lst' parameter is a list of tuples. The last item in each tuple
must be a VariantCombination. The rest of each tuple is the "piece of
information" discussed above."""
line 1215 - 'require any' should be 'require-any' right?
Sure
line 1270 - I don't see why you're skipping the order of the
is_successor check from the previous version of the code? If this is
correct, then does the comment at 1266 still hold?
Do you mean switching the order? The comment on 1266 still holds, the
code was just wrong previously. Here, comp_fmri is the target fmri of a
require dependency. c is coming from the set of fmris from the
require-any dependency currently being checked for redundency. So, we
can drop the require-any dependency if there's already a require
dependency on a successor to c. Ie, if the require-any depends on a@1,
b@2 c@3, then a require dependency on c@4 means we can drop the
require-any dependency, but not a require dependency on c@2.
line 1383 - extra space in the block comment I think
Oops, yep.
Thanks for the review, if I switch to named tuples, I'll send another
webrev out, otherwise I'll wait for convergence on the comment content
for line 950 before I put back.
Brock
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
