Edward Pilatowicz wrote:
> > > https://cr.opensolaris.org/action/browse/pkg/edp/pkg.pli.cr1/webrev/
>
> > - line 3916 et al: while you don't need to create a new ImagePlan object
> > here, you can still keep the shortening to ip. -- fewer changes, and
> > better readability (IMO, at least where you use it twice; less so in
> > __calc_frozen()).
> >
>
> (i think this comment applies to image.py)
> i'm not sure i understand the comment.
>
> do you want me to change:
> import pkg.client.imageplan as imageplan
> to:
> import pkg.client.imageplan as ip
>
> and then rename any variables called "ip" to "imageplan"?
No. The code used to be
ip = imageplan.ImagePlan( ... )
self._avoid_set_save(... set(ip.match(pat_list, ip.MATCH)))
but now that you've put it all into a single statement (because you don't
need an actual ImagePlan object, then you end up repeating
"imageplan.ImagePlan" a bunch of times in the self._avoid_set_save() call.
I was simply suggesting that you do
ip = imageplan.ImagePlan
self._avoid_set_save(... | set(ip.match(self, pat_list, ip.MATCH)))
> > imageplan.py:
> >
> > - line 186: the comment expression isn't particularly useful here, and
> > it'll hide the rest of the assertion. Either ditch it or enhance it.
> >
>
> i can't match the line numbers up, but i'm assume you were referring to
> the following lame comment (that i've removed):
>
> # can't skip preexecute since we already preexecuted it
No, that wasn't it. You have
assert self.pd.state in [ ... ], self.pd.state
If and when that assertion fails, you'll simply get
AssertionError: 3
which isn't terribly useful. But I think I was wrong -- the rest of the
stack trace will have the code that failed. I still think the message
(i.e., self.pd.state) should be a bit more informative than simply the one
value.
> > pkgremote.py:
> >
> > - line 166: why use real files?
>
> what would you recommend in place of real files?
>
> i don't want to use pipes because unless i create threads to constantly read
> and drain data from the pipes, clients could block while writing output to
> stdout.
Right; we discussed this in the hallway, and I simply forgot to remove the
comment from the review.
> > - line 550: shouldn't need frozenset here.
> >
>
> why don't i need frozenset? a frozenset is not the same as a set:
> ---8<---
> >>> isinstance(frozenset(), set)
> False
> ---8<---
Sorry, didn't realize that. How freakin' weird.
Thanks,
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
