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

[Adam Young]

On 03/31/2011 03:12 PM, Adam Young wrote:
> 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
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
Hadn't merged in the changes.  Updated patch attached.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/9bf3a2ff/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0219-2-code-review-fixes.patch
Type: text/x-patch
Size: 14218 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/9bf3a2ff/attachment.bin>