Looks good to me. Thanks. Jean
On 12/ 3/10 07:56 AM, Darren Kenny wrote:
Hi Jean, Thanks for the review, I've redone some of these changes as you describe, could you please take another look at them - I've updated the webrev. Thanks, Darren. On 12/ 2/10 05:10 PM, jean.mccormack wrote:I'll review the transfer module code: cpio.py: line 593: destination is required here so I would put in the not_found_is_err=True flag . That means you can get rid of lines 595, 598, 599 lines 602-607: ditto ips.py: lines 706-716 I think something like this is what I envisioned origin_list = pub.get_children(Origin.ORIGIN_LABEL, Origin) if origin_list: if preferred: or_repo = origin.origin else: or_repo = publisher.RepositoryURI(uri=origin.origin) origin_name.append(or_repo) self.logger.debug(" Origin Info: %s", origin.origin) else: origin_name = self.DEF_REPO_URI line 751: put the not_found_err flag in here and remove the check at 754 Jean On 12/ 2/10 09:09 AM, Darren Kenny wrote:Hi, I'd like to request a code review for the following bug: 7003533 get_descendents should return an empty list when no objects are found - http://monaco.sfbay.sun.com/detail.jsf?cr=7003533 The webrev can be found at: http://cr.opensolaris.org/~dkenny/fix.7003533.slim/ This represents a change in the API for the DataObjectBase/DataObject methods: get_descendants() get_children() find_path() delete_children() In each case, an additional parameter has been added called 'not_found_is_err' which is a boolean value that, if: - True Will generate an ObjectNotFoundError if an attempt to locate an object which doesn't exist. - False (Now the *Default*) Where there is a value returned, which is the case for all except delete_children(), then if nothing is found to match the criteria, it will return an empty list - []. This will require anyone depending on the ObjectNotFoundError to be generated to modify their code since the default (as requested) is to NOT fire the exception by default... I modified any code that I could find that makes use of this exception, either directly or just by non-action with it - so I'd like to really ensure good review of this by the owners of these areas - you know who you are :) When doing these changes, I tried to make the logic simpler, but if there were cases where values weren't being tested, and as such people were relying on the fact an exception was being thrown, then I didn't change the code too much, and only added the not_found_error=True parameter, since it would be a higher risk when I'm unfamiliar with the code in question. I added tests for with/without the value being set in the install_doc, and I ran the existing PyUnit tests in slim to ensure there was no change in behaviour in other modules. Thanks, Darren. PS - it may be worth the CUD/DC gate applying this change and running some tests in their workspace too, to see if it has any impact there... _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
