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

Reviewers: mp+189970_code.launchpad.net,

Message:
Please take a look.

Description:
Make the charm source tab work better

This was a quick hack I did that I wanted to land rather than throw
away. Quick hacks are a lot less quick once you throw in tests. :-/

https://code.launchpad.net/~gary/juju-gui/bug1203583/+merge/189970

(do not edit description out of merge proposal)

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

Affected files (+100, -2 lines):
   A [revision details]
   M app/models/charm.js
   M app/subapps/browser/views/bundle.js
   M app/subapps/browser/views/charm.js
   M test/test_browser_charm_details.js
   M test/test_model.js

https://codereview.appspot.com/14565044/diff/1/app/subapps/browser/views/bundle.js
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14565044/diff/1/app/subapps/browser/views/bundle.js#newcode169
app/subapps/browser/views/bundle.js:169: 'subapp-browser-entitybaseview'
I discovered that the file needed these in order for some test files to
be run individually.

https://codereview.appspot.com/14565044/

LGTM QA OK with trivial

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

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode466
app/models/charm.js:466: setter: function(value) {
you can quote this instead of commenting it if you like.

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode468
app/models/charm.js:468: value.sort(function(a, b) {
This all looks great but I'm curious what this sort method gets you over
the standard lexicographic sort. You may also want to add this comment
to the code so others know when looking at this.

https://codereview.appspot.com/14565044/

*** Submitted:

Make the charm source tab work better

This was a quick hack I did that I wanted to land rather than throw
away. Quick hacks are a lot less quick once you throw in tests. :-/

R=jeff.pihach
CC=
https://codereview.appspot.com/14565044

https://codereview.appspot.com/14565044/

On 2013/10/08 21:03:10, jeff.pihach wrote:
> LGTM QA OK with trivial

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

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode466
> app/models/charm.js:466: setter: function(value) {
> you can quote this instead of commenting it if you like.

https://codereview.appspot.com/14565044/diff/1/app/models/charm.js#newcode468
> app/models/charm.js:468: value.sort(function(a, b) {
> This all looks great but I'm curious what this sort method gets you
over the
> standard lexicographic sort. You may also want to add this comment to
the code
> so others know when looking at this.

Thank you for the review and comments, Jeff. I added the following
explanation to the code.

             // This sort has several properties that are different than
a
             // standard lexicographic sort.
             // * Filenames in the root are grouped together, rather than
             // sorted along with the names of directories.
             // * Sort ignores case, unless case is the only way to
             // distinguish between values, in which case it is honored
             // per-directory. For example, this is sorted: "a", "b/c",
             // "b/d", "B/b", "B/c", "e/f". Notice that "b" directory
values
             // and "B" directory values are grouped together, where
they
             // would not necessarily be in a simpler case-insensitive
             // lexicographic sort.
             // If you rip this out for something different and simpler,
that's
             // fine; just don't tell me about it. :-) This seemed like
a good
             // approach at the time.

https://codereview.appspot.com/14565044/