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

[Adam Young]

On 01/26/2011 04:32 PM, Adam Young wrote:
> On 01/26/2011 12:37 PM, Adam Young wrote:
>> 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
>>
>>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
> Rebased
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110127/909df7fe/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-admiyo-0154-3-declarative-defintions.patch
Type: text/x-patch
Size: 41258 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110127/909df7fe/attachment.bin>