Merge lp://staging/~gary/juju-gui/simplifycharmstore into lp://staging/juju-gui/experimental

Reviewers: mp+131086_code.launchpad.net,

Message:
Please take a look.

Description:
Last change round of charm store data structures

This change is hopefully the last round of changes, at least for a long
while, to the underlying charm store infrastructure. It is more deletes
than additions, and changes the code to take advantage of the changes
Kapil made to the charm store.

I made some decisions as to how to factor some of these things and would
be happy to describe my rationale for changes if desired. I'm thinking
particularly of the immediate creation of charm objects as search
results. That allowed for some simplifications and I think is mostly a
win.

The sorting code is simplified yet again.

Thanks

Gary

https://code.launchpad.net/~gary/juju-gui/simplifycharmstore/+merge/131086

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6733067/

Affected files:
   A [revision details]
   M app/app.js
   M app/models/charm.js
   M app/modules.js
   M app/store/charm.js
   M app/templates/charm-search-result.handlebars
   M app/views/charm-search.js
   M test/data/search_results.json
   M test/data/series_search_results.json
   M test/test_app.js
   M test/test_charm_configuration.js
   M test/test_charm_search.js
   M test/test_charm_store.js
   M test/test_model.js
   M test/test_service_view.js

Even though this is a large branch I think it removes more code than it
adds which is always a plus. I had some minor feedback but this LGTM.

I'd still rather Kapil get a chance to look this over but I think the
general architectural issues that were present before (and which I
didn't take into account either) are no longer present.

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode56
app/models/charm.js:56: this.on('load', function() { this.loaded = true;
});
Don't you need to either use self here or pass this as context to the
'on' call?

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode84
app/models/charm.js:84: options.get_charm(
Why the two naming styles on these_twoMethods? get_charm, loadByPath?

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-result.handlebars
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-result.handlebars#newcode13
app/templates/charm-search-result.handlebars:13: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
I thought you didn't need the #if when there is no content other than
the var which can default to null, no?

https://codereview.appspot.com/6733067/

*** Submitted:

change charm store data structures

This change is hopefully the last round of changes, at least for a long
while, to the underlying charm store infrastructure. It is more deletes
than additions, and changes the code to take advantage of the changes
Kapil made to the charm store.

The sorting code is simplified yet again.

R=benjamin.saller
CC=
https://codereview.appspot.com/6733067

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode56
app/models/charm.js:56: this.on('load', function() { this.loaded = true;
});
On 2012/10/26 07:56:19, benjamin.saller wrote:
> Don't you need to either use self here or pass this as context to the
'on' call?
No, because the listener is on "this" so the context is what I want. I
already have a test that verifies, and I doublechecked in the chromium
debugger as well.

https://codereview.appspot.com/6733067/diff/1/app/models/charm.js#newcode84
app/models/charm.js:84: options.get_charm(
On 2012/10/26 07:56:19, benjamin.saller wrote:
> Why the two naming styles on these_twoMethods? get_charm, loadByPath?
As we discussed, it's because we haven't standardized one way or the
other across all our files. Within a file, we are consistent. This
uses get_charm, from the older env js, and loadByPath, from the newer
charm store js. I got your agreement in person that this is fine.
OTOH I switched charmIDRe and idElements in this file, based on your
reminder.

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-result.handlebars
File app/templates/charm-search-result.handlebars (right):

https://codereview.appspot.com/6733067/diff/1/app/templates/charm-search-result.handlebars#newcode13
app/templates/charm-search-result.handlebars:13: {{#if
owner}}{{owner}}/{{/if}}{{package_name}}</a>
On 2012/10/26 07:56:19, benjamin.saller wrote:
> I thought you didn't need the #if when there is no content other than
the var
> which can default to null, no?
Yes, but I have the slash after the owner.

https://codereview.appspot.com/6733067/

Thank you for the review, Ben. I have comments in-line, largely in line
with what we discussed in person.

You gave me permission to go ahead and land this. Kapil mentioned that
he questioned the use of the Model load/sync code given that it is of
minimal value to our use case, but if I need to address this I will do
it in a subsequent branch.

Thanks

Gary

https://codereview.appspot.com/6733067/