Merge lp:~makyo/juju-gui/recess into lp:juju-gui/experimental

[Madison Scott-Clary]

Use recess as CSS linter

This is the first step to implementing a CSS linter (recess). Currently, most of the options are turned off in order to keep this slack task short, so our LESS file passes the linter. Further slack tasks may be created for options that we decide to turn on down the road (personal suggestions: noOverqualifying and zeroUnits, ambivalent about strictPropertyOrder).

https://codereview.appspot.com/7067057/

[Madison Scott-Clary (makyo)]

Reviewers: mp+142572_code.launchpad.net,

Message:
Please take a look.

Description:
Use recess as CSS linter

This is the first step to implementing a CSS linter (recess).
Currently, most of the options are turned off in order to keep this
slack task short, so our LESS file passes the linter. Further slack
tasks may be created for options that we decide to turn on down the road
(personal suggestions: noOverqualifying and zeroUnits, ambivalent about
strictPropertyOrder). There is one work-around in place in
lib/templates.js due to https://github.com/twitter/recess/issues/22 in
that recess is called twice - once to lint and once to compile - in
order to side-step an OOM error. This is still quite fast, and not used
in production.

https://code.launchpad.net/~makyo/juju-gui/recess/+merge/142572

(do not edit description out of merge proposal)

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

Affected files:
   M Makefile
   A [revision details]
   M lib/templates.js
   M package.json
   A recess.json

Index: Makefile
=== modified file 'Makefile'
--- Makefile 2012-12-21 12:52:30 +0000
+++ Makefile 2013-01-09 18:36:08 +0000
@@ -38,7 +38,7 @@
   node_modules/minimatch node_modules/mocha node_modules/node-markdown \
   node_modules/node-minify node_modules/node-spritesheet \
   node_modules/rimraf node_modules/should node_modules/yui \
- node_modules/yuidocjs
+ node_modules/yuidocjs node_modules/recess
  EXPECTED_NODE_TARGETS=$(shell echo "$(NODE_TARGETS)" | tr ' ' '\n' | sort \
   | tr '\n' ' ')

@@ -223,7 +223,10 @@
  yuidoc-lint: $(JSFILES)
   bin/lint-yuidoc

-lint: gjslint jshint yuidoc-lint
+recess: node_modules/recess
+ recess lib/views/stylesheet.less --config recess.json | grep -q Perfect
+
+lint: gjslint jshint recess yuidoc-lint

  virtualenv/bin/gjslint virtualenv/bin/fixjsstyle:
   virtualenv virtualenv

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: package.json
=== modified file 'package.json'
--- package.json 2013-01-09 16:42:50 +0000
+++ package.json 2013-01-09 17:23:33 +0000
@@ -24,6 +24,7 @@
      "chai": ">=1.2.0",
      "less": "1.3.x",
      "jshint": ">=0.9.1",
+ "recess": "1.1.x",
      "node-markdown": "0.1.x",
      "yuidocjs": "0.3.x",
      "minimatch": "0.2.x",

Index: recess.json
=== added file 'recess.json'
--- recess.json 1970-01-01 00:00:00 +0000
+++ recess.json 2013-01-09 18:36:08 +0000
@@ -0,0 +1,7 @@
+{
+ "compile": false,
+ "noIDs": false,
+ "noOverqualifying": false,
+ "strictPropertyOrder": false,
+ "zeroUnits": false
+}

Index: lib/templates.js
=== modified file 'lib/templates.js'
--- lib/templates.js 2012-12-20 21:59:21 +0000
+++ lib/templates.js 2013-01-09 18:37:46 +0000
@@ -5,7 +5,7 @@
      exec = require('child_process').exec,
      YUI = require('yui').YUI,
      view = require('./view.js'),
- less = require('less'),
+ recess = require('recess'),
      config = require('../config.js').config,
  ...

[Gary Poster (gary)]

land with changes.

Please apply the diff I gave, or do something similar. You don't need
to do anything about lib/templates.js. Let's stick with what you have.

Thank you!

Gary

https://codereview.appspot.com/7067057/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7067057/diff/1/Makefile#newcode227
Makefile:227: recess lib/views/stylesheet.less --config recess.json |
grep -q Perfect
This runs a global "recess" and not the one installed locally.
http://pastebin.ubuntu.com/1514675/ has a fix for this.

https://codereview.appspot.com/7067057/diff/1/Makefile#newcode465
Makefile:465: undocumented yuidoc yuidoc-lint
recess is a "PHONY" target because there is no "recess" file in the root
of the build. The pastebin also has this change.
http://pastebin.ubuntu.com/1514675/

https://codereview.appspot.com/7067057/diff/1/lib/templates.js
File lib/templates.js (right):

https://codereview.appspot.com/7067057/diff/1/lib/templates.js#newcode191
lib/templates.js:191: });
I wonder about this change, but I am OK with it. Simply using less and
relying on recess for later linting might be nicer. OTOH, maybe this
keeps us from falling over when the LESS file is bad.

https://codereview.appspot.com/7067057/

[Nicola Larosa (teknico)]

Land with (Gary's) changes.

No comments in addition to Gary's ones, this looks great, thanks.

https://codereview.appspot.com/7067057/

[Madison Scott-Clary (makyo)]

Thanks for the reviews.

https://codereview.appspot.com/7067057/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7067057/diff/1/Makefile#newcode227
Makefile:227: recess lib/views/stylesheet.less --config recess.json |
grep -q Perfect
On 2013/01/09 22:10:24, gary.poster wrote:
> This runs a global "recess" and not the one installed locally.
> http://pastebin.ubuntu.com/1514675/ has a fix for this.

Good catch! Had it installed globally for testing and had forgotten.
Applied the patch.

https://codereview.appspot.com/7067057/diff/1/lib/templates.js
File lib/templates.js (right):

https://codereview.appspot.com/7067057/diff/1/lib/templates.js#newcode191
lib/templates.js:191: });
On 2013/01/09 22:10:24, gary.poster wrote:
> I wonder about this change, but I am OK with it. Simply using less
and relying
> on recess for later linting might be nicer. OTOH, maybe this keeps us
from
> falling over when the LESS file is bad.

I reverted this change. recess just uses less to compile, and if the
LESS file is bad, the error we get is the error from the less package.
No real value added.

https://codereview.appspot.com/7067057/

[Madison Scott-Clary (makyo)]

*** Submitted:

Use recess as CSS linter

This is the first step to implementing a CSS linter (recess).
Currently, most of the options are turned off in order to keep this
slack task short, so our LESS file passes the linter. Further slack
tasks may be created for options that we decide to turn on down the road
(personal suggestions: noOverqualifying and zeroUnits, ambivalent about
strictPropertyOrder).

R=gary.poster, teknico
CC=
https://codereview.appspot.com/7067057

https://codereview.appspot.com/7067057/

=== modified file 'Makefile'
--- Makefile	2012-12-21 12:52:30 +0000
+++ Makefile	2013-01-09 18:46:30 +0000
@@ -38,7 +38,7 @@
 	node_modules/minimatch node_modules/mocha node_modules/node-markdown \
 	node_modules/node-minify node_modules/node-spritesheet \
 	node_modules/rimraf node_modules/should node_modules/yui \
-	node_modules/yuidocjs
+	node_modules/yuidocjs node_modules/recess
 EXPECTED_NODE_TARGETS=$(shell echo "$(NODE_TARGETS)" | tr ' ' '\n' | sort \
 	| tr '\n' ' ')
 
@@ -223,7 +223,10 @@
 yuidoc-lint: $(JSFILES)
 	bin/lint-yuidoc
 
-lint: gjslint jshint yuidoc-lint
+recess: node_modules/recess
+	recess lib/views/stylesheet.less --config recess.json | grep -q Perfect
+
+lint: gjslint jshint recess yuidoc-lint
 
 virtualenv/bin/gjslint virtualenv/bin/fixjsstyle:
 	virtualenv virtualenv
 
=== modified file 'lib/templates.js'
--- lib/templates.js	2012-12-20 21:59:21 +0000
+++ lib/templates.js	2013-01-09 18:46:30 +0000
@@ -5,7 +5,7 @@
     exec = require('child_process').exec,
     YUI = require('yui').YUI,
     view = require('./view.js'),
-    less = require('less'),
+    recess = require('recess'),
     config = require('../config.js').config,
     cache = {};
 
@@ -159,21 +159,36 @@
   stylesheet: {
     output: __dirname + '/../build-shared/juju-ui/assets/juju-gui.css',
     callback: function(strategy, name) {
-      var parser = new less.Parser({
-        paths: [config.server.view_dir],
-        filename: 'stylesheet.less'
-      }),
-          css_data = fs.readFileSync(
-          path.join(config.server.view_dir, 'stylesheet.less'), 'utf8');
-      parser.parse(css_data, function(e, tree) {
-        if (e) {
-          console.log('LESS Generation Error', e);
-          return;
-        }
-        fs.writeFileSync(strategy.output,
-            tree.toCSS({compress: true}));
-      });
+      // Lint the file without compiling using our config first.
+      var recessConfig = JSON.parse(
+          fs.readFileSync(__dirname + '/../recess.json'));
+      recess(
+          path.join(config.server.view_dir, 'stylesheet.less'),
+          recessConfig,
+          function(err, obj) {
+            if (err) {
+              console.log('LESS Generation Error', err);
+              return;
+            }
+            // Warn of lint errors.
+            console.log(obj.errors);
+          });
 
+      // Compile the less to the output file without worrying about our config.
+      // This is due to a memory-leak in recess when multiple options are
+      // specified with compile=true.
+      // See: https://github.com/twitter/recess/issues/22
+      recess(
+          path.join(config.server.view_dir, 'stylesheet.less'),
+          { compile: true },
+          function(err, obj) {
+            if (err) {
+              console.log('LESS Generation Error', err);
+              return;
+            }
+            // Write to output.
+            fs.writeFileSync(strategy.output, obj.output);
+          });
     }
   }
 };
 
=== modified file 'package.json'
--- package.json	2013-01-09 16:42:50 +0000
+++ package.json	2013-01-09 18:46:30 +0000
@@ -24,6 +24,7 @@
     "chai": ">=1.2.0",
     "less": "1.3.x",
     "jshint": ">=0.9.1",
+    "recess": "1.1.x",
     "node-markdown": "0.1.x",
     "yuidocjs": "0.3.x",
     "minimatch": "0.2.x",
 
=== added file 'recess.json'
--- recess.json	1970-01-01 00:00:00 +0000
+++ recess.json	2013-01-09 18:46:30 +0000
@@ -0,0 +1,7 @@
+{
+  "compile": false,
+  "noIDs": false,
+  "noOverqualifying": false,
+  "strictPropertyOrder": false,
+  "zeroUnits": false
+}