On 03/31/2011 04:29 PM, Adam Young wrote:
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@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Including Automount


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@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@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
This version uses a spec for the details facet, IAW code review feedback


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
ACKed after lengthy discussion and minor fix up in IRC

Patches 217 and 219 pushed to master
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to