Review: Disapprove
I hinted at it yesterday, I'll say it in full today: the renaming of
Widget#stop to Widget#destroy brings absolutely no value and no justifications,
does not make anything clearer and gratuitously breaks 6.1 code. Even on
aesthetic grounds it is indefensible since it breaks the current symmetry
between Widget#start and Widget#stop. Worse, it implies destructor or finalizer
semantics which don't exist (the object still exists in a known state after the
end of #stop, and there are little to no restrictions on what it can do in its
own #stop).
For the rest, why not with two caveats:
* Not fond of the Java getter/setter patterns in Java, even less so in
javascript
* `openerp.web.CallbackEnabled.extend(openerp.web.ParentedMixin).extend(other)`
creates some sort of subclass to CallbackEnabled then re-extends it immediately
and throws it out
* Why the double-underscore prefixes when a single underscore will do?
* _.each(this.getChildren(), function(el) {
el.destroy();
});
why not _.invoke?
* _.each(_.clone(this.getChildren()), function(el) {
el.destroy();
});
why clone the children collection (which was already cloned before being
returned, too) when you're not altering it, merely stopping all the children?
* if (!parent.getChildren())
will not work correctly, and neither will the implementation of
#getChildren(): empty arrays are truty in js, so there is no way the code will
ever enter this conditional.
* furthermore, on the line below `parent.getChildren().push(this);` just wastes
a bunch of cycle: #getChildren() never returns the actual childrens array, it
either returns a brand new empty array or returns a shallow clone of the
childrens array in the parent, so this line 1. creates a new array with the
parent's children (#getChildren); 2. adds an element to the array (#push); 3.
throws the edited array in the garbage. It's a noop as far as the parent object
is concerned.
* Why are there start/stop semantics independent of the parent/child
relationship in the "ParentedMixin"? Namely, what do `__parented_stopped` and
`isDestroyed` have to do with parent/child relationships which the mixin name
loosely implies is its job?
--
https://code.launchpad.net/~openerp-dev/openerp-web/trunk-core-extraction-1-niv/+merge/94021
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openerp-web/trunk-core-extraction-1-niv.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
