+1 from our side. David can we delay the build until Danek has a chance to review the updated webrev?
Thanks, JR Michal Pryc wrote: > Danek, > Thanks for comments much appriciated. > > http://cr.opensolaris.org/~migi/18_10_2008_bug_5172_v4/ > > >>> It's this what you were suggesting? >>> http://cr.opensolaris.org/~migi/18_10_2008_bug_5172_v2/ >>> >> Yes, that's it. Except please get rid of the parentheses around the >> exception names. That's only necessary if you have more than one. And >> please get rid of the backslashes inside parens where you're adding or >> changing code. >> > I did that for the touched parts of the code. > > >> I don't follow your statement about how it's similar to something in >> image.py -- there's nothing there that looks for either EDQUOT or ENOSPC. >> Can you be more specific? >> > 1213 + if uex.errno in (errno.EDQUOT, > errno.ENOSPC): > > is similar to line 901: > http://src.opensolaris.org/source/xref/pkg/gate/src/modules/client/image.py?r=723 > > >> My remaining concern is that you're catching all exceptions on line 1224, >> which is usually a mistake. We've concentrated our catching of Exception >> to the top level, as lower down it's usually too large an umbrella. Can >> you explain why you do it this way here? >> > JR>> For the UM when a general exception like this happens, we need to > catch it at this level, adjust the progress staus and output the > information to the progress dialog details panel. This is the > appropriate place for us to handle all the errors for the Update All > progress step. > > >> A couple of other nits: >> >> - line 1226: _() encompasses what_msg, as well as the hard-coded string. >> Is that intentional? It means that for every possible "what_msg", >> there has to be a translation of "<what_msg> unexpected error:", which >> doesn't seem right. >> > This change was added by Takao and there was an reason for localization > (I don't know what was the reason). At this stage we will not change > this since we are on the RC2 build and only stoppers can be fixed. > > >> - line 1226: no need for parens around what_msg >> > > best > Michal > _______________________________________________ > pkg-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
