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

[Adam Young]

On 01/27/2011 12:12 PM, Adam Young wrote:
> 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
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
ACKed in IRC by Edewata.  Pushed to master
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110127/a4455e7b/attachment.htm>