[Freeipa-devel] [PATCH] 0270-removing-setters-setup-and-init

[Adam Young]

On 07/25/2011 08:18 PM, Endi Sukma Dewata wrote:
> I think we should remove IPA.spacer_widget and leave the 
> IPA.dnszone_adder_dialog unchanged for now. The current code, although 
> it might not be very concise, is done correctly and does produce the 
> desired layout (label on the right of the checkbox and a space below 
> it). The IPA.spacer_widget on the other hand violates the design and 
> still doesn't produce all the intended layout. Also, chances are the 
> priority for fixing this afterward will be low. Suppose we change the 
> IPA.widget to verify that all widgets have a name, this code will fail 
> immediately. 
Can't:  the code you had working was based on the split of a couple of 
the functions, and now the pieces needed from the base class are not 
split out.  Getting this to work again will require either significant 
code duplication or reworking of the DNS class, and neither is 
preferable to a short lived space_widget.  The best we can do is remove 
the space_widget and not have any visual separation, and then do the 
visual separation in a different patch. I didn't want to change what you 
had working in this patch, but I had to do something to get it to work 
again.



For the rest of the review issues, I think I would prefer the other 
option: "move the code to the bottom of the class and mark the area as 
constructor."  However sometimes some code needs to be done prior to the 
baseclass call:


modify_spec(spec);

var that = IPA.base_class(spec);

.
.
.


And then, in many places, we do


that.create = function(container){...}


If we moved the code to the bottom, we'd have to do

IPA.somthing = function(spec){
     function create(container){}
     //constructor
     modify_spec(spec);
     var that = IPA.base_class(spec);
     that.create = create;
}


Which is probably a better coding practice, but would change a lot of code.


our coding standard should probably be:

NAMESPACE.function_name = function(spec){


//private variable declaration.

//private member functions, to include function bodies that will be made 
public at the end


//code that must run prior to baseclass call

that = IPA.baseclass(spec);

//public variables

that.var_name = spec.var_name || '';



//public function definitions:

that.create = create;


}

I'd like to get away from how we do super functions, too, but that is 
probably too ambitious.  I feel like our current approach is fragile, 
and bucks the language, but the alternative is to use prototype 
inheritance, and again, that would change a lot of code, and the super 
method calling is not standardized anyway.

I think that all the changes you have enumerated fall into the realm of 
"we need a standard but one has not been set yet."  I'd like to get this 
patch in as is, and meanwhile we can hammer out what that standard 
should be.