On Thu 02 Aug 2012 at 04:13PM, Brock Pytlik wrote:
> On 08/02/12 15:24, Dan Price wrote:
> >Hi,
> >
> >Here's a review for one more bug I want to fix in the
> >progress/printengine stuff. The fix here is again quite small-- in
> >short, we are switching away from using os.isatty(filehandle.fileno())
> >to filehandle.isatty(). This should fix the problems induced in the
> >install gate.
> >
> >The rest of the fix is devoted to hooking up more testing infrastructure
> >related to printengine and progress tracking. The nice upside is that
> >the 'interactive' test utilities I added, runprogress and runprintengine,
> >now have test coverage.
> >
> >https://cr.opensolaris.org/action/browse/pkg/dp/pkg-install-fix
> >
> >Thanks,
> >
> > -dp
> >
> printengine.py:
> 51: I don't think "PrintEngineException" should be in this string,
> or this shouldn't be an API exception. The expectation of error
> messages for API exceptions is that they can/will be presented to
> the user, so including that kind of internal only info doesn't seem
> appropriate to me.
Ok. I made PrintEngine and Progress exceptions be API exceptions
I think because someone told me in a previous review that I'd go
to coding jail if they weren't. Previously these were API exceptions
that didn't have any messages at all. I personally don't care what
they are.
> 104: Could this error message be more helpful to a user? I guess if
> I saw this, I'm not sure what I'd do to correct things.
Yeah these aren't really intended for display to users. I'll think
about what to do.
> 311: Debugging leftover?
I thought it was a clarifying comment, but I can rip it out.
> progress.py:
> 61: Same comment about ProgressTrackerException
> 2114,2115: Same comment as line 104 in printengine.py
>
> 2124,2125: Do we not care about windows any more, or is this somehow
> covered in the existing code in a way I don't understand?
I got rid of this because self.cr is not used anywhere. It's possible
that the printengine will need work if we want to resuscitate windows,
but it's pretty clear to me that this code isn't in the right place, so
I nuked it.
-dp
--
Daniel Price, Solaris Kernel Engineering
_______________________________________________
pkg-discuss mailing list
pkg-discuss@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
