On 06/12/2013 11:40 AM, Tomas Babej wrote: > On 06/12/2013 10:23 AM, Alexander Bokovoy wrote: >> On Mon, 10 Jun 2013, Ana Krivokapic wrote: >>>> And once here(added by your patch): >>>> >>>> + if not self.validate_range(*keys, **options): >>>> + raise errors.ValidationError( >>>> + name=_('id range'), >>>> + error=_( >>>> + 'An id range already exists for this trust. ' >>>> + 'You should either delete the old range, or ' >>>> + 'exclude --base-id/--range-size options from the >>>> command.' >>>> >>>> I'd suggest we have one common place, say validate_range() function as >>>> we have now, >>>> that contains all the checks (more coming in the future for sure). >>>> >>>> That would mean that the whole trust-add command will fail in the case >>>> that "ID range >>>> with the same name but different domain SID already exists", since we >>>> now call validate_range() >>>> within execute() method. I checked with Alexander and we agreed that >>>> this is more desired behaviour. >>>> >>>> This would also result to reducement of the number of API calls, which >>>> is a nice benefit. >>>> >>>> Tomas >>> >>> This updated patch completely separates validation logic and object >>> creation logic of the trust_add command. I added two new methods: >>> >>> * validate_range(), which ensures that the options and environment >>> related to idrange is valid, and >>> * validate_options, which takes care of other general validation >>> >>> One change introduced in this patch is making the >>> __populate_remote_domain() method of TrustDomainJoins class public, and >>> calling it from trust_add. This was necessary in order to enable >>> discovering details of the trusted domain in validation phase. >> Looks good overall but I'd suggest to wrap populate_remote_domain() >> calls in join_ad_* methods instead of removing them. If remote domain is >> not initialized (i.e. >> not(isinstance(self.remote_domain,TrustedDomainInstance)), >> then call populate_remote_domain() as it was before. >> > I tested the patch and it works fine. > > There's a lot of refactoring done, but the changes preserve equal state. Aside > from > Alexander's comment up here, I have but one nitpick. > > There are few constructs of the form: > > try: > value = dictionary['keyword'] > except KeyError: > value = None > > or > > if 'keyword' in dictionary: > value = dictionary['keyword'] > else: > value = None > > Can you please substitute these with "value = dictionary.get(keyword, None)"? > Not everywhere, but there are places where it can improve readability of the > code.
You can even use just "value = dictionary.get(keyword)" as None is the default return value: >>> print {}.get("foo") None Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel