Code review comment for lp://staging/~gary/juju-gui/makefile

Reviewers: mp+132302_code.launchpad.net,

Message:
Please take a look.

Description:
'make server' should not always run yuidoc

I discovered that 'make server' always ran yuidoc. This was because, in
part, the automatically generated FILES list included inappropriate
files. There is another factor that affects this that I did not
identify, but fixing the first problem is sufficient.

Fixing the problem specified the JS files more tightly, which revealed a
few more lint changes.

I did a fly-by in the README.

https://code.launchpad.net/~gary/juju-gui/makefile/+merge/132302

(do not edit description out of merge proposal)

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

Affected files:
   M Makefile
   M README
   A [revision details]
   M app/assets/javascripts/svg-layouts.js
   M test-server.js

Index: Makefile
=== modified file 'Makefile'
--- Makefile 2012-10-31 08:30:13 +0000
+++ Makefile 2012-10-31 10:57:16 +0000
@@ -1,4 +1,5 @@
-FILES=$(shell bzr ls -RV -k file | grep -v assets/ | grep -v
app/templates.js | grep -v server.js)
+JSFILES=$(shell bzr ls -RV -k file | grep -E -e '.+\.js(on)?$$|
generateTemplates$$' | grep -Ev -e '^manifest\.json$$' -e '^test/assets/'
-e '^app/assets/javascripts/reconnecting-websocket.js$$' -e '^server.js$$')
+
  # After a successful "make" run, the NODE_TARGETS list can be regenerated
with
  # this command (and then manually pasted in here):
  # find node_modules -maxdepth 1 -mindepth 1 -type d
-printf 'node_modules/%f '
@@ -20,12 +21,14 @@
  app/templates.js: $(TEMPLATE_TARGETS) bin/generateTemplates
   @./bin/generateTemplates

-yuidoc: install $(FILES)
+yuidoc/index.html: node_modules/yuidocjs $(JSFILES)
   @node_modules/.bin/yuidoc -o yuidoc -x assets app

+yuidoc: yuidoc/index.html
+
  $(SPRITE_GENERATED_FILES): node_modules/grunt
node_modules/node-spritesheet $(SPRITE_SOURCE_FILES)
   @node_modules/grunt/bin/grunt spritegen
- mv bin/sprite app/assets/sprite/
+ @mv bin/sprite app/assets/sprite/

  $(NODE_TARGETS): package.json
   @npm install
@@ -38,12 +41,12 @@
  gjslint: virtualenv/bin/gjslint
   @virtualenv/bin/gjslint --strict --nojsdoc --jslint_error=all \
       --custom_jsdoc_tags
module,main,class,method,event,property,attribute,submodule,namespace,extends,config,constructor,static,final,readOnly,writeOnce,optional,required,param,return,for,type,private,protected,requires,default,uses,example,chainable,deprecated,since,async,beta,bubbles,extension,extensionfor,extension_for
\
- $(FILES)
+ $(JSFILES)

  jshint: node_modules/jshint
- @node_modules/jshint/bin/hint $(FILES)
+ @node_modules/jshint/bin/hint $(JSFILES)

-yuidoc-lint: $(FILES)
+yuidoc-lint: $(JSFILES)
   @bin/lint-yuidoc

  lint: gjslint jshint yuidoc-lint
@@ -53,7 +56,7 @@
   @virtualenv/bin/easy_install archives/closure_linter-latest.tar.gz

  beautify: virtualenv/bin/fixjsstyle
- @virtualenv/bin/fixjsstyle --strict --nojsdoc --jslint_error=all $(FILES)
+ @virtualenv/bin/fixjsstyle --strict --nojsdoc --jslint_error=all
$(JSFILES)

  spritegen: $(SPRITE_GENERATED_FILES)

@@ -87,4 +90,4 @@
  appcache-force: appcache-touch appcache

  .PHONY: test lint beautify server install clean prep jshint gjslint \
- appcache appcache-touch appcache-force spritegen yuidoc-lint
+ appcache appcache-touch appcache-force yuidoc spritegen yuidoc-lint

Index: README
=== modified file 'README'
--- README 2012-10-30 14:04:32 +0000
+++ README 2012-10-31 10:57:16 +0000
@@ -9,6 +9,10 @@
    $ sudo apt-get update
    $ sudo apt-get install nodejs npm

+You also need ImageMagick.
+
+ $ sudo apt-get install imagemagick
+
  The linter will be installed locally per branch, but if you want editor
  integration, you may want to install jshint globally in your system::

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: test-server.js
=== modified file 'test-server.js'
--- test-server.js 2012-10-02 11:29:56 +0000
+++ test-server.js 2012-10-31 10:57:16 +0000
@@ -6,22 +6,22 @@
      path = require('path');

-server.configure(function () {
- server.use(express.logger('dev'));
- // 'static' is a reserved word so dot notation is not used to
- // avoid annoying the linter.
- server.use(express['static'](__dirname));
- // fallback to looking in assets
- server.use('/juju-ui', express['static'](__dirname + '/app/'));
- server.use(express.bodyParser());
- server.use(express.methodOverride());
+server.configure(function() {
+ server.use(express.logger('dev'));
+ // 'static' is a reserved word so dot notation is not used to
+ // avoid annoying the linter.
+ server.use(express['static'](__dirname));
+ // fallback to looking in assets
+ server.use('/juju-ui', express['static'](__dirname + '/app/'));
+ server.use(express.bodyParser());
+ server.use(express.methodOverride());
  });

  var port = 8084;

-server.listen(port, function () {
- console.log('Server listening on ' + port);
+server.listen(port, function() {
+ console.log('Server listening on ' + port);
  });

Index: app/assets/javascripts/svg-layouts.js
=== modified file 'app/assets/javascripts/svg-layouts.js'
--- app/assets/javascripts/svg-layouts.js 2012-09-14 04:35:36 +0000
+++ app/assets/javascripts/svg-layouts.js 2012-10-31 10:57:16 +0000
@@ -1,16 +1,18 @@
-YUI.add("svg-layouts", function(Y) {
- // package to help compute svg layouts,
- // particuallary around text
- Y.Node.addMethod("getClientRect", function(node) {
- // Chrome and FF both support this at the DOM level
- var rect = node.getClientRects()[0];
- if (typeof(rect) == "undefined" || !rect.width) {
- return null;
- }
- return rect;
- });
-
-
-}, "0.0.1", {
- requires: ["node"]
+'use strict';
+
+YUI.add('svg-layouts', function(Y) {
+ // package to help compute svg layouts,
+ // particuallary around text
+ Y.Node.addMethod('getClientRect', function(node) {
+ // Chrome and FF both support this at the DOM level
+ var rect = node.getClientRects()[0];
+ if (typeof(rect) === 'undefined' || !rect.width) {
+ return null;
+ }
+ return rect;
+ });
+
+
+}, '0.0.1', {
+ requires: ['node']
  });