[Freeipa-devel] [PATCH] 414-415 Move spec modifications from facet factories to pre_ops

[Petr Vobornik]

On 05/16/2013 02:01 PM, Petr Vobornik wrote:
> On 05/15/2013 04:02 PM, Ana Krivokapic wrote:
>> On 05/15/2013 12:09 PM, Petr Vobornik wrote:
>>> [PATCH] 414 Move spec modifications from facet factories to pre_ops
>>> -------------------------------------------------------------------
>>>
>>> Spec modifications in factories makes inheritance and extensibility
>>> more difficult.
>>>
>>> Moving them to pre_ops allows modification of their output by other
>>> pre_ops.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3605
>>>
>>> [PATCH] 415 Unite and move facet pre_ops to related modules
>>> -----------------------------------------------------------
>>>
>>> Facet pre_ops defined in ./facet module were moved to modules where
>>> facet are actually defined. Moved pre_ops were united with the ones
>>> defined for the facets in these modules.
>>>
>>> The move simplifies module dependencies - there is no reason to have
>>> general facet module dependent on specialized facet modules.
>>>
>>> Pre_ops uniting makes the code simpler.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3605
>>>
>>>
>>
>> A minor nitpick - in patch 415:
>>
>> +        if (indirect_members) {
>> +            if (indirect_members.indexOf(spec.other_entity) > -1) {
>> +                return true;
>> +            }
>> +        }
>>
>> This construct:
>> if (A) {
>>      if (B) {
>>          do_something();
>>      }
>> }
>>
>> Can be replaced with:
>> if (A && B) {
>>      do_something();
>> }
>>
>> The second option is more readable, and avoids nested ifs.
>
> Changed:
> -        if (indirect_members) {
> -            if (indirect_members.indexOf(spec.other_entity) > -1) {
> -                return true;
> -            }
> -        }
> -        return false;
> +        var has_indirect = !!(indirect_members &&
> indirect_members.indexOf(spec.other_entity) > -1);
> +        return has_indirect;
>
>
> Updated patch attached.
>
>>
>>
>> I have found no other issues, so ACK from me.
>>
>> --
>> Regards,
>>
>> Ana Krivokapic

Pushed to master, ipa-3-2.
-- 
Petr Vobornik