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.

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to