[Freeipa-devel] [PATCH] 0270-removing-setters-setup-and-init
Adam Young
ayoung at redhat.com
Tue Jul 26 01:59:19 UTC 2011
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.
More information about the Freeipa-devel
mailing list