Updated webrev at: http://cr.opensolaris.org/~bpytlik/ips-4053-v2/
Danek Duvall wrote: > On Fri, Oct 17, 2008 at 09:09:00PM -0700, Brock Pytlik wrote: > > >> http://cr.opensolaris.org/~bpytlik/ips-4053-v1/ >> > > api_errors.py: > > - line 101: I'm confused by this -- it doesn't add anything, unless > self.unfound_fmris is ever not an iterable (like, say, None). List > comprehensions that iterate over an empty list are themselves empty > lists, so the previous incarnation of this code shouldn't have been > doing anything to res with an empty self.unfound_fmris except create an > empty list. > > It looks like in both cases that the exception is raised, a list, even > if empty, is passed in for unfound and multiple. One case passes None > in for missing, but that could be fixed. > You're right, I managed to confuse myself when I looked at this last week. However, I'm going to make a change similar to the one Shawn's making for 4080. In case there's a question about why I didn't make the same change to missing_matches it's because the string for unfound_fmris covers two lines and contains a fair amount of information. > image.py: > > - line 2175: Do we really need the if here? Either calling inventory() > raises InventoryException (so catch it and set unfound) or it succeeds, > and you can set missing. > Ok. > Also, what happens if inventory() raises InventoryException because of > an illegal FMRI. Does the messaging make sense then? > I'll make the assertion that the messaging makes as much, or little, sense as it did before this change. This change as offered simply makes it so that missing_matches can actually be populated since, as you noted above, matches can never be an empty list. That said, I've updated the change to take illegal fmris into account and added a case for that for the PlanCreationException. Thanks for the review, Brock > Danek > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
