[Freeipa-devel] [PATCH] admiyo-0154-declarative-defintions

[Adam Young]

Rebased on top of origin/master, and made changes.  See comments below.


On 01/20/2011 02:48 PM, Endi Sukma Dewata wrote:
> On 1/20/2011 11:10 PM, Adam Young wrote:
>> If you ACK, please don't push, but let me do so, as it will likely
>> conflict with other UI work.
>
> There is no major issues, just some comments:
>
> 1. The declarative definition is a bit inconsistent. Some methods like 
> association() takes a spec, but other methods like facet() takes an 
> object instance.
>
>         association({
>             'name': 'netgroup',
>             'associator': 'serial'
>         }).
>         facet(
>             IPA.search_facet({
>                 'name': 'search',
>                 'label': 'Search'
>             }).

The difference is for things that are created self contained, like 
association, and things like search and details facets that require 
additional declaration.  We could change the association call to require 
creating the association, but not the other way around.

Aside: there should be no need to speficy name or label for search and 
details.

>
> 2. The diff tool uses the first line of the function to mark the 
> chunks like this:
>
>   @@ -593,10 +593,7 @@ IPA.permission = function () {
>
> Having a function name in the first line would make it easier to read. 
> Compare this definition:
>
>   IPA.permission = function () {
>
> with this definition:
>
>   IPA.register_entity(function () {

Even better, we can use an associative array, and do the two at once.


>
> 3. The following lines (webui.js:128-133):
>
>     IPA.start_entities();
>
>     for (var i=0; i<IPA.entities.length; i++) {
>         var entity = IPA.entities[i];
>         entity.init();
>     }
>
> probably could be combined into a single method:
>
>     IPA.init_entities();
Done

>
> I think this method name will make more sense.
>
> 4. Entity's init_dialogs() probably could be merged into entity.init().

Done
>
> 5. The entity_factories is probably better be named entity_classes. 
> Factory is usually an object that creates multiple other objects. The 
> entity 'factory' is really the entity class which is only instantiated 
> once.

Nah, Factory can create only a singe instance.  Classes is too loaded a 
term.
>
> 6. Typo on search.js:258:
>
>     spec.label = spec.lable ||  IPA.messages.facets.search;

Fixed

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0154-1-declarative-defintions.patch
Type: text/x-patch
Size: 42547 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110126/b6021c42/attachment.bin>