Simple maven plugin that applies the caja innocent code transformer.

This maven plugin addresses the following issues in a apache shindig:
https://issues.apache.org/jira/browse/SHINDIG-1293
https://issues.apache.org/jira/browse/SHINDIG-1294

[Jasvir, 2010-03-07 18:59:26]

http://codereview.appspot.com/273041/diff/1/2
File plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java
(right):

http://codereview.appspot.com/273041/diff/1/2#newcode19
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:19:
package caja.maven.plugin;
What project is this a patch against?  Caja or Shindig or something else?

http://codereview.appspot.com/273041/diff/1/2#newcode94
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:94:
scanner.setExcludes(new String[]{"**/*.opt.js", "**/*.taming.js"});
Do you mean **/taming.js?

[chirag, 2010-03-07 21:42:38]

http://codereview.appspot.com/273041/diff/1/2
File plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java
(right):

http://codereview.appspot.com/273041/diff/1/2#newcode19
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:19:
package caja.maven.plugin;
The patch is against Caja. 
SVN Base: http://google-caja.googlecode.com/svn/trunk/

On 2010/03/07 18:59:26, jasvir wrote:
> What project is this a patch against?  Caja or Shindig or something else?

http://codereview.appspot.com/273041/diff/1/2#newcode94
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:94:
scanner.setExcludes(new String[]{"**/*.opt.js", "**/*.taming.js"});
Good catch. Fixed
On 2010/03/07 18:59:26, jasvir wrote:
> Do you mean **/taming.js?

[Jasvir, 2010-03-08 00:24:36]

http://codereview.appspot.com/273041/diff/1/2
File plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java
(right):

http://codereview.appspot.com/273041/diff/1/2#newcode19
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:19:
package caja.maven.plugin;
Ok this is against Caja.  Please use com.google.caja.tools as the package and
linewrap at 80 columns.

http://codereview.appspot.com/273041/diff/1/2#newcode37
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:37: *
@goal ict
Suggest using "innocent" as the transform goal to be consistent with
InnocentAntTask.

http://codereview.appspot.com/273041/diff/1/2#newcode72
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:72:
throw new Exception("Transformation failed");
Can we throw MojoFailureException here instead?

[chirag, 2010-03-08 08:26:10]

http://codereview.appspot.com/273041/diff/1/2
File plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java
(right):

http://codereview.appspot.com/273041/diff/1/2#newcode19
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:19:
package caja.maven.plugin;
On 2010/03/08 00:24:36, jasvir wrote:
> Ok this is against Caja.  Please use com.google.caja.tools as the package and
> linewrap at 80 columns.
> 
> 

Done.

http://codereview.appspot.com/273041/diff/1/2#newcode37
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:37: *
@goal ict
On 2010/03/08 00:24:36, jasvir wrote:
> Suggest using "innocent" as the transform goal to be consistent with
> InnocentAntTask.

Done.

http://codereview.appspot.com/273041/diff/1/2#newcode72
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:72:
throw new Exception("Transformation failed");
On 2010/03/08 00:24:36, jasvir wrote:
> Can we throw MojoFailureException here instead?

Done.

[Jasvir, 2010-03-08 16:51:20]

http://codereview.appspot.com/273041/diff/1/2
File plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java
(right):

http://codereview.appspot.com/273041/diff/1/2#newcode19
plugins/caja-ict-plugin/src/main/java/caja/maven/plugin/CajaIctMojo.java:19:
package caja.maven.plugin;
Please include Maven jars in third party as part of this CL.  See
http://codereview.appspot.com/277043/show for example.

On 2010/03/08 08:26:11, chirag wrote:
> On 2010/03/08 00:24:36, jasvir wrote:
> > Ok this is against Caja.  Please use com.google.caja.tools as the package
and
> > linewrap at 80 columns.
> > 
> > 
> 
> Done.

http://codereview.appspot.com/273041/diff/9001/3002
File
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java
(right):

http://codereview.appspot.com/273041/diff/9001/3002#newcode58
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java:58:
File srcFile = new File(sourceDirectory + "/" + fileName);
Use File.pathSeparator instead of "/" here and else where.

http://codereview.appspot.com/273041/diff/9001/3002#newcode69
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java:69:
fileName.replace(".js", ".ict.js"));
Hmm.  If fileName contains a .js in the name other than at the end - for example
in the directory part or in the middle of the name - it'll rename the file
incorrectly (eg. a.js.js).

http://codereview.appspot.com/273041/diff/9001/3002#newcode69
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java:69:
fileName.replace(".js", ".ict.js"));
You're relying on fileName containing .js (otherwise fileName gets clobbered). 
This is true because of the way processFile is called but its non-obvious. 
Suggest adding an assert.

http://codereview.appspot.com/273041/diff/9001/3002#newcode100
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java:100:
scanner.setExcludes(new String[]{"**/*.opt.js", "**/taming.js"});
Perhaps also exclude **/*.ict.js.

[Jasvir, 2010-05-18 17:55:30]

ping!

[chirag, 2010-05-18 17:58:33]

http://codereview.appspot.com/273041/diff/9001/3002
File
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java
(right):

http://codereview.appspot.com/273041/diff/9001/3002#newcode58
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java:58:
File srcFile = new File(sourceDirectory + "/" + fileName);
On 2010/03/08 16:51:20, jasvir wrote:
> Use File.pathSeparator instead of "/" here and else where.

Done.

http://codereview.appspot.com/273041/diff/9001/3002#newcode69
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java:69:
fileName.replace(".js", ".ict.js"));
On 2010/03/08 16:51:20, jasvir wrote:
> You're relying on fileName containing .js (otherwise fileName gets clobbered).

> This is true because of the way processFile is called but its non-obvious. 
> Suggest adding an assert.

Done.

http://codereview.appspot.com/273041/diff/9001/3002#newcode100
plugins/caja-ict-plugin/src/main/java/com/google/caja/tools/CajaIctMojo.java:100:
scanner.setExcludes(new String[]{"**/*.opt.js", "**/taming.js"});
On 2010/03/08 16:51:20, jasvir wrote:
> Perhaps also exclude **/*.ict.js.

Done.

[Jasvir, 2010-05-27 18:12:07]

http://codereview.appspot.com/273041/diff/14001/15002
File pom.xml (right):

http://codereview.appspot.com/273041/diff/14001/15002#newcode1
pom.xml:1: <?xml version="1.0" encoding="UTF-8"?>
Should this file should be in the maven directory?

http://codereview.appspot.com/273041/diff/14001/15002#newcode41
pom.xml:41: <version>r3950</version>
I am not too familiar with maven.  Is it possible to not have to specify a
particular version of Caja here but to use the same one as specified by the caja
pom.xml?

http://codereview.appspot.com/273041/diff/14001/15001
File src/main/java/com/google/caja/tools/CajaIctMojo.java (right):

http://codereview.appspot.com/273041/diff/14001/15001#newcode41
src/main/java/com/google/caja/tools/CajaIctMojo.java:41: public class
CajaIctMojo extends AbstractMojo {
Since this is the general maven plugin point for Caja, I suggest calling it
CajaMojo or something else not ICT specific.

http://codereview.appspot.com/273041/diff/14001/15001#newcode105
src/main/java/com/google/caja/tools/CajaIctMojo.java:105:
scanner.setIncludes(new String[]{"**/*.js"});
Can the includes and excludes also be configurable in the pom as the source
directory is - perhaps with some defaults.

[chirag, 2010-06-03 04:55:06]

http://codereview.appspot.com/273041/diff/14001/15002
File pom.xml (right):

http://codereview.appspot.com/273041/diff/14001/15002#newcode41
pom.xml:41: <version>r3950</version>
I've designed the plugin to be standalone, and decoupled from other top-level
maven artifacts.

 
On 2010/05/27 18:12:08, jasvir wrote:
> I am not too familiar with maven.  Is it possible to not have to specify a
> particular version of Caja here but to use the same one as specified by the
caja
> pom.xml?

http://codereview.appspot.com/273041/diff/14001/15001
File src/main/java/com/google/caja/tools/CajaIctMojo.java (right):

http://codereview.appspot.com/273041/diff/14001/15001#newcode41
src/main/java/com/google/caja/tools/CajaIctMojo.java:41: public class
CajaIctMojo extends AbstractMojo {
On 2010/05/27 18:12:08, jasvir wrote:
> Since this is the general maven plugin point for Caja, I suggest calling it
> CajaMojo or something else not ICT specific.

Done.

http://codereview.appspot.com/273041/diff/14001/15001#newcode105
src/main/java/com/google/caja/tools/CajaIctMojo.java:105:
scanner.setIncludes(new String[]{"**/*.js"});
On 2010/05/27 18:12:08, jasvir wrote:
> Can the includes and excludes also be configurable in the pom as the source
> directory is - perhaps with some defaults.

Done.