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

Endi Sukma Dewata edewata at redhat.com
Thu Mar 31 16:51:59 UTC 2011


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.

2. There's a IPA.metadata assignment and the like in unit tests, this
    is redundant.

3. In IPA.entity_builder.section(), the current_section should be added
    to the current_facet before adding the fields to do incremental
    construction.

4. In IPA.entity_builder the entity_name can be replaced with
    entity.name to reduce the number of variables.

5. In IPA.entity_builder the standard_associations() can be replaced
    standard_association_facets() for consistency.

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.

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.

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.

9. The IPA.entity_builder could be a singleton because it doesn't take
    any parameters and there's no multi-threading issue.

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.

-- 
Endi S. Dewata




More information about the Freeipa-devel mailing list