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#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?
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 endpoints. js (right):
File app/models/
https:/ /codereview. appspot. com/8275043/ diff/2001/ app/models/ endpoints. js#newcode12 endpoints. js:12: var models = Y.namespace( 'juju.models' );
app/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 endpoints. js:15: models. endpoints_ map = {};
app/models/
camelCase?
https:/ /codereview. appspot. com/8275043/ diff/2001/ app/models/ endpoints. js#newcode169 endpoints. js:169: // var requires = et.get( 'requires' );
app/models/
evt.currentTarg
commented out old code that should be removed?
https:/ /codereview. appspot. com/8275043/ diff/2001/ app/models/ endpoints. js#newcode255 endpoints. js:255: if (true) { // Avoid lint warning.
app/models/
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 endpoints. js:256: var rel = {};
app/models/
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 models. js (right):
File app/models/
https:/ /codereview. appspot. com/8275043/ diff/2001/ app/models/ models. js#newcode563 models. js:563: this.endpoints_map = {};
app/models/
camelCase?
https:/ /codereview. appspot. com/8275043/ diff/2001/ app/views/ topology/ relation. js topology/ relation. js (right):
File app/views/
https:/ /codereview. appspot. com/8275043/ diff/2001/ app/views/ topology/ relation. js#newcode563 topology/ relation. js:563: var endpoints = getEndpoints( service, models. endpoints_ map, db);
app/views/
models.
camelCase endpoints_map
https:/ /codereview. appspot. com/8275043/ diff/2001/ test/test_ endpoints. js endpoints. js (right):
File test/test_
https:/ /codereview. appspot. com/8275043/ diff/2001/ test/test_ endpoints. js#newcode135 endpoints. js:135: var charm = new models.Charm({id: wordpress- 2'});
test/test_
'cs:precise/
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/