Code review comment for lp:~benji/charms/precise/juju-gui/use-npm-cache

Thanks for the docs and comments! They are great to have.

29 + # If the given installable thing ("backend") requires one or more debs
30 + # that are not yet installed, install them.
31 missing = backend.check_packages(*backend.debs)

Yeah, check_packages might be better named install_missing_packages or something like that.

39 + # Inject NPM packages into the cache for faster building.
40 + #prime_npm_cache(get_npm_cache_archive_url())

Uh? I hope this wasn't commented out for your tests. :-)

44 + # XXX Why do we only set up the GUI if the "juju-gui-source"
45 + # configuration is non-default?

It's not non-default: it is not-what-it-was-before--or at least that's the intent. I think that's the behavior too, or we'd be in trouble. :-) At the start, what-it-was-before is some empty value, I believe.

124 + if e.errno != 17: # File exists.

Maybe use errno.EEXIST instead?

126 + uncompress = command('tar', '-x', '-z', '-C', npm_cache_dir, '-f')
127 + cmd_log(uncompress(npm_cache_archive))

That reads very nicely. Good idea.

164 + get_npm_cache_archive_url,

You look like you were on your way to a test? That would have been nice. :-) If not, remove the import. But adding an import of prime_npm_cache and testing them both would be nicer. :-)

Thanks!

Gary