Looks good, Matt. Two small suggestions. Thanks! Gary
https://codereview.appspot.com/6776052/diff/9005/Makefile File Makefile (right): https://codereview.appspot.com/6776052/diff/9005/Makefile#newcode58 Makefile:58: appcache-force: appcache-touch appcache I'm guessing you added this because you discovered you needed it, so that's cool if so. That said, I don't see a reason to factor out appcache-touch though. Oh wait...I see. It's an ordering issue. OK, new, different suggestion: add a comment explaining what is going on so people don't have to figure it out. :-) https://codereview.appspot.com/6776052/diff/9005/Makefile#newcode60 Makefile:60: .PHONY: test lint beautify server install clean prep jshint gjslint appcache appcache-touch appcache-force If you already know this, forgive me, but if you wanted to you could make those break at 80 chars or similar by having a trailing "\" at the end of lines that break. Something like .PHONY: test lint beautify server install clean prep jshint gjslint \ appcache appcache-touch appcache-force https://codereview.appspot.com/6776052/ -- https://code.launchpad.net/~makyo/juju-gui/make-appcache/+merge/131586 Your team Juju GUI Hackers is requested to review the proposed merge of lp:~makyo/juju-gui/make-appcache into lp:juju-gui. -- Mailing list: https://launchpad.net/~yellow Post to : [email protected] Unsubscribe : https://launchpad.net/~yellow More help : https://help.launchpad.net/ListHelp
