Code review comment for lp:~bac/juju-gui/get-endpoints

Revision history for this message
Jeff Pihach (hatch) wrote :

This is looking great Thanks for all this work! It really cleans up a
lot of code.

I'd like to open up a discussion between a few of us to see if this
should be moved into it's own closure and then the rest of the comments
are pretty trivial.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js
File app/models/endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode12
app/models/endpoints.js:12: var models = Y.namespace('juju.models');
I know that this is how it was done previously but it feels like these
methods are just hanging off of juju.models with all of the other models
when they should really be within their own closure of utility methods
to clean up the object. This should also make it easier to test.
Thoughts?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode15
app/models/endpoints.js:15: models.endpoints_map = {};
camelCase?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode169
app/models/endpoints.js:169: // var requires =
evt.currentTarget.get('requires');
commented out old code that should be removed?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode255
app/models/endpoints.js:255: if (true) { // Avoid lint warning.
Is this required so that the linter doesn't complain about the vars
being re-declared below?

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode256
app/models/endpoints.js:256: var rel = {};
I'm surprised that the linter didn't complain about these vars being
re-declared every loop. Typically `k`, `rel`, `j` would be defined at
the top of the fn with `result` then re-assigned in every loop.

https://codereview.appspot.com/8275043/diff/2001/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/models.js#newcode563
app/models/models.js:563: this.endpoints_map = {};
camelCase?

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation.js#newcode563
app/views/topology/relation.js:563: var endpoints =
models.getEndpoints(service, models.endpoints_map, db);
camelCase endpoints_map

https://codereview.appspot.com/8275043/diff/2001/test/test_endpoints.js
File test/test_endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/test/test_endpoints.js#newcode135
test/test_endpoints.js:135: var charm = new models.Charm({id:
'cs:precise/wordpress-2'});
Because this attaches events we should destroy it at the end of every
test and make sure that the events that it attaches are destroyed as
well.

https://codereview.appspot.com/8275043/

« Back to merge proposal