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

Hi Rick. Thank you very much for the review. I have a few points I'd like to negotiate on. Please see below.

On Sep 7, 2013, at 7:36 AM, Richard Harding <email address hidden> wrote:

> LGTM with notes below. Will QA in a sec.
>
>
> https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js
> File app/views/databinding.js (right):
>
> https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode62
> app/views/databinding.js:62: return (normalizedValue === currentValue);
> should we refactor this out into any _stringEq or something since it's
> duped in the three eq methods?

Yeah, I agree we need something like that. I'll think on it and do something like this before landing.

>
> https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode688
> app/views/databinding.js:688: if (nodeHandler.eq(e.target,
> binding.get(model))) {
> will this blow up if no binding was found matching key?

Yes, but my argument is that, in that case, we have a pre-existing problem, because we should be explicitly running an addBinding for every binding we have, as I read the code. Therefore I decided to go for the "explicit error we can debug more easily rather than a silent error we'll never know about" route. Do you agree with this, but request that I add a comment to this effect; or disagree, and argue for more defensive code here?

>
> https://codereview.appspot.com/13385045/diff/1/app/views/databinding.js#newcode875
> app/views/databinding.js:875: 'event-valuechange']
> is this required in this module? It's used in the inspector I believe,
> but not here.

This module subscribes to the valueChanged event. I think the dependency rightly belongs here, because of that. Perhaps it would be the right thing to do to remove the dependency in the inspector module, given my argument. WDYT?

>
> https://codereview.appspot.com/13385045/diff/1/test/test_databinding.js
> File test/test_databinding.js (right):
>
> https://codereview.appspot.com/13385045/diff/1/test/test_databinding.js#newcode26
> test/test_databinding.js:26: 'model', 'model-list',
> 'node-event-simulate'];
> this needs the event-valueChange since you are binding with that event
> below.

Not if the module I test (databinding) itself depends on this, IMO. I only agree with you if I revert the event-valuechange dependency in databinding, but I think that the dependency arrangement I have in my branch is better than the one in trunk in this regard. Agree?

Thanks again!

Gary

>
> https://codereview.appspot.com/13385045/
>
> --
> https://code.launchpad.net/~gary/juju-gui/databindingChangedValues/+merge/184407
> You are the owner of lp:~gary/juju-gui/databindingChangedValues.