[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