Thanks again Johansen for reviewing and adding in your valuable
comments, please read my in-line reply.........
[email protected] wrote:
On Fri, May 28, 2010 at 06:11:16PM +0530, Saurabh Vyas wrote:
Question : Do we still need to log the errors here ? Or just raising
the exception would do ?
I'm not certain. We want to make sure that the exception is saved in
the pkg history, but I don't know if that requires the logger for that
to happen or not. Shawn would know for sure.
New Webrev : http://cr.opensolaris.org/~saurabhv/fix-10437-rev2/
client.py:
- You haven't caught all of the cases where this can be raised.
Double-check to make sure that you've found all of the places were
we call imageplan.prepare(). It looks to me like the image.repair()
code calls imageplan.prepare() too.
Thanks for this info, I have added the exception handing here too.
Also as explained by John Rice, the exception handling for GUI is
already taken care by
the base class ApiException, so I need not to do anything specific there.
api_errors.py:
- lines 653-655: If you're always going to pass all of the arguments
to the function, do you really need to declare keyword arguments
here?
- line 665, 669, 674, 679, and 681: This could be simplified if you
set:
res = ""
then you can do:
res += s
instead, and just
return res
Done.
As all the args are passed here, I have taken off the keyword arguments.
progress.py:
- lines 212-218: Same question as with api_errors: If you're always
passing all arguments, do you really need keyword arguments here?
Done, the new webrev : http://cr.opensolaris.org/~saurabhv/fix-10437-rev3/
General questions:
- How have you tested this code? Would you please add some api tests
to ensure that this works now, and isn't broken later on? There
don't appear to be any api tests for the progress tracker right now,
so you may have to devise an API module that tests different parts
of the progress tracking module.
Sure I would add up the test cases here, but should that be part of this
bug-fix itself ? (as a new test module for progress-tracker is to be added)
Or should I raise another bug for that ? (as I am not sure about what
all scenarios needs to be tested ).
I would be interested in working on this new test module, also I would
need some more inputs for the same.
Thanks again,
~Saurabh
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
