[Freeipa-devel] [PATCH] admiyo-0217-define-entities-using-builder-and-more-declarative

[Adam Young]

On 03/31/2011 02:09 PM, Adam Young wrote:
> On 03/31/2011 12:51 PM, Endi Sukma Dewata wrote:
>> On 3/31/2011 8:43 AM, Adam Young wrote:
>>> rebased both patches
>>
>> I don't see any code change in the rebased patches, only new commit 
>> ID's, I hope this is correct.
>>
>> Some comments (some of which had been discussed over IRC):
>>
>> 1. I ran my Selenium test cases against 215, 216, and 217 together,
>>    so far there's no failure.
> Good to know.
>
>>
>> 2. There's a IPA.metadata assignment and the like in unit tests, this
>>    is redundant.
>
> yeah, I suspect it was from before I explicitly set async.  removed.
>
>>
>> 3. In IPA.entity_builder.section(), the current_section should be added
>>    to the current_facet before adding the fields to do incremental
>>    construction.
> Done.
>
>>
>> 4. In IPA.entity_builder the entity_name can be replaced with
>>    entity.name to reduce the number of variables.
>
> Done
>>
>> 5. In IPA.entity_builder the standard_associations() can be replaced
>>    standard_association_facets() for consistency.
> Done
>>
>> 6. In the permission entity definition, the 'add_fields' is used
>>    inconsistently to add a section (i.e. IPA.target_section). The
>>    solution is either adding 'add_sections' or converting
>>    IPA.target_sections into widgets. I think adding 'add_sections' is
>>    simpler because widgets is designed to represent a single attribute.
>
> Gonna punt on this for this patch.  Not certain on the correct 
> approach, either to make adders have sections, or to convert the one 
> custom section we have to a widget.  Either way, beyond the scope of 
> this patch, and it will only affect one entity and the builder when we 
> decide.
>
>>
>> 7. The IPA.entity_builder.details_facet() takes an array of sections
>>    instead of a spec object. This limits the expandability of the
>>    builder interface. It should take a spec object with a 'sections'
>>    attribute containing the array of sections, this would be consistent
>>    with the other interfaces.
>
> I thought about it, but there is nothing that we want to customize in 
> the details_facet.  We can always do a custom facet to customize.  An 
> alternative is to check the type of that is passed in to the 
> details_section method on the builder, check if it is an object or an 
> array, and treat it like a spec or array depending.
>
>>
>> 8. In IPA.entity_builder.search_facet(), there's no need to call
>>    current_facet.init() because all facets will be initialized by
>>    the entity when IPA.start_entities() is invoked.
>
> Done.
>
>>
>> 9. The IPA.entity_builder could be a singleton because it doesn't take
>>    any parameters and there's no multi-threading issue.
> Going to leave it like this for now.  That change would be limited to 
> a single file (entity.js) and can be done if we decide we want to with 
> a very small patch.
>
>
>>
>> 10. In IPA.details_refresh() it calls IPA.refresh_devel_hook()
>>     to execute a code specifically used by testing. I think ideally
>>     the develop.js should modify the entity_builder instance to generate
>>     details facet with test-specific code. This requires item #9. But
>>     for now at least we should rename IPA.refresh_devel_hook() into
>>     IPA.details_refresh_devel_hook() for clarity.
>
> Good idea.  Done
>
>
>  I had already submitted patch freeipa-admiyo-0218-default-all-false.  
> Some of the fixes here would conflict with that patch if rebased.  
> What I've attached is on top of 217-2 and 218-1.
>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
Including Automount

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/8924a49b/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0219-1-code-review-fixes.patch
Type: text/x-patch
Size: 13675 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/8924a49b/attachment.bin>