Code review comment for lp:~huwshimi/juju-gui/bundle-charm-list-updates

Revision history for this message
Richard Harding (rharding) wrote :

O>

https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/bundle.js#newcode120
> app/subapps/browser/views/bundle.js:120:
> _model is a pretty confusing name, IMO. Why not simply
service._charm? Also,
> this is using an underline effectively as a namespace tool, which I
don't love.

We have to be careful not to collide with keys that could occur in the
service block in the deployer file section. Using the _ is a
"collision-avoider". What would you prefer as an alternative?

https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/bundle.js#newcode138
> app/subapps/browser/views/bundle.js:138:
> The 'entity' is a bundle here, right? It is called 'entity' because
we are
> sharing code with a charm view as well, right, so in other files
'entity' is a
> charm? If so, please add a comment explaining this (at least the fact
that
> this.get('entity') is a bundle).

Yep, the details shares a lot of common code in entity-base.js which was
refactored out to share between charm and bundle details views.

https://codereview.appspot.com/14920057/

« Back to merge proposal