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

[Adam Young]

On 03/31/2011 03:27 PM, Adam Young wrote:
> 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.
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
This version uses a spec for the details facet, IAW code review feedback
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/b5f365c3/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0219-3-code-review-fixes.patch
Type: text/x-patch
Size: 30095 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110331/b5f365c3/attachment.bin>