On Mon, Jun 18, 2012 at 09:33:42AM -0700, Danek Duvall wrote:
> 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)))
>
ah. ok. i updated the three places where i had two consecutive
references to imageplan.ImagePlan to use a temporary variable like
you have above:
---8<---
--- a/src/modules/client/image.py
+++ b/src/modules/client/image.py
@@ -3922,17 +3922,19 @@ in the environment or by setting simulat
names; ignore versions."""
with self.locked_op("avoid"):
+ ip = imageplan.ImagePlan
self._avoid_set_save(self.avoid_set_get() | set(
- imageplan.ImagePlan.match_user_stems(self,
pat_list,
- imageplan.ImagePlan.MATCH_UNINSTALLED)))
+ ip.match_user_stems(self, pat_list,
+ ip.MATCH_UNINSTALLED)))
def unavoid_pkgs(self, pat_list, progtrack, check_cancel):
"""Unavoid the specified packages... use pattern matching on
names; ignore versions."""
with self.locked_op("unavoid"):
- unavoid_set = set(imageplan.ImagePlan.match_user_stems(
- self, pat_list, imageplan.ImagePlan.MATCH_ALL))
+ ip = imageplan.ImagePlan
+ unavoid_set = set(ip.match_user_stems(self,
+ pat_list, ip.MATCH_ALL))
current_set = self.avoid_set_get()
not_avoided = unavoid_set - current_set
if not_avoided:
@@ -4025,9 +4027,9 @@ in the environment or by setting simulat
# Match the user's patterns against the frozen packages
# and return the stems which matched, and the
dictionary
# of the currently frozen packages.
- return set(imageplan.ImagePlan.match_user_stems(self,
- pat_list, imageplan.ImagePlan.MATCH_ALL,
- raise_unmatched=False,
+ ip = imageplan.ImagePlan
+ return set(ip.match_user_stems(self, pat_list,
+ ip.MATCH_ALL, raise_unmatched=False,
universe=[(None, k) for k in d.keys()])), d
if dry_run:
---8<---
> > > 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.
>
i've updated the assert to say:
assert self.pd.state in \
[plandesc.PREEXECUTED_OK, plandesc.EVALUATED_OK], \
"%s not in [%s, %s]" % (self.pd.state,
plandesc.PREEXECUTED_OK, plandesc.EVALUATED_OK)
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
