Code review comment for lp://staging/~gary/juju-gui/databindingChangedValues

On 2013/09/09 13:30:01, benji wrote:
> This branch looks good, yet I found ways to complain anyway.

:-) Thank you for the review. Comments below.

> LGTM

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js
> File app/views/databinding.js (right):

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#newcode37
> app/views/databinding.js:37: var _textEq = function(node, value) {
> The leading underscore strikes me as odd.

Yeah, on reflection, I agree.

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#newcode56
> app/views/databinding.js:56: return !! value;
> This is just a nitpick: I don't think we normally have a space between
the
> double negation and the variable name.

Yeah, this was conscious because I think the "!!" is an odd "cast to
bool" operator but prefer it to an explicit Boolean(). I thought the
space might call it out a bit better, but yeah, on reflection agree that
my thought is not particularly compelling and I should stick to
precedent.

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#newcode78
> app/views/databinding.js:78: var _indexBindings = function(bindings,
keyfunc,
> multiple) {
> Another leading underscore. Maybe there a pattern I don't recognize.

<shrug>. As before, agreed with you and removed.

https://codereview.appspot.com/13385045/diff/5001/app/views/databinding.js#newcode679
> app/views/databinding.js:679: binding = b;
> Tricky. Maybe too tricky.

Heh. I dunno. I thought about and decided to keep it: it seemed like
it is or will soon become an idiomatic pattern, once "some" is in a
greater percentage of existing browsers. In deference to your
reasonable concern, though, I did add a comment that might help a reader
in the future to know what is going on. Maybe.

https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js
> File test/test_databinding.js (right):

https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js#newcode596
> test/test_databinding.js:596: done();
> Darn. I guess this is the best we can do.

Yeah, apparently. :-/ I could have tried to write a narrower test
instead, but stuck with the pre-existing, more integrated approach.

https://codereview.appspot.com/13385045/diff/5001/test/test_databinding.js#newcode677
> test/test_databinding.js:677: // of full. The system should allow for
this
> As a reward for making this bad comment better I will complain that it
isn't
> perfect:

> - It's unnecessary (and slightly confusing) to have "dep" instead of
(I assume)
> "dependency".

> - The first sentence is good enough for a period, but the second... no
way!

:-P OK, OK, I made these changes and a couple more I thought helped. :-)

Thanks again!

https://codereview.appspot.com/13385045/