Support for Google Closure Compiler

[RishikeshDarandale]

Hi David, I have added support for Google closure compiler to your plugin. This is first time I am contributing to any open source, please review the changes and let me know your review comments.

Thanks Rishi

[davidB]

Thanks, I'll take a look ASAP.

On Sun, May 5, 2013 at 7:35 AM, RishikeshDarandale <notifications@github.com

wrote:

Hi David, I have added support for Google closure compiler to your plugin. This is first time I am contributing to any open source, please review the changes and let me know your review comments.

Thanks

Rishi

You can merge this Pull Request by running

git pull https://github.com/RishikeshDarandale/yuicompressor-maven-plugin master

Or view, comment on, or merge it at:

https://github.com/davidB/yuicompressor-maven-plugin/pull/62 Commit Summary

• Added support for using google closure compiler

File Changes

• M pom.xmlhttps://github.com/davidB/yuicompressor-maven-plugin/pull/62/files#diff-0(9)
• A src/main/java/net_alchim31_maven_googleclosure/GoogleClosureMojo.javahttps://github.com/davidB/yuicompressor-maven-plugin/pull/62/files#diff-1(143)
• M src/main/java/net_alchim31_maven_yuicompressor/YuiCompressorMojo.javahttps://github.com/davidB/yuicompressor-maven-plugin/pull/62/files#diff-2(134)

Patch Links:

• https://github.com/davidB/yuicompressor-maven-plugin/pull/62.patch
• https://github.com/davidB/yuicompressor-maven-plugin/pull/62.diff

[davidB]

Sorry for late reply, I was busy.

I dislike to reject contribution (especialy for a first time), but I'll not merge your PR.

Why ?

• first I thought cool, but it's off-topic:
  • in this case the plugin will be misnamed
  • there is already few maven's plugins for closure-compiler on search.maven.org or wro4j
• I don't want to force user to download both jar (current user will have to download useless 3.3Mo), this can be "workarounded" by defining the dependency as optional and by downloading the missing jar at runtime.
• I don't understand why you split the Mojo in an abstract class and a single concret class that take care of both compressor (bad name of the class) ?
• the compressors don't support the same set of configuration's parameters, so they should expose the specific.
• My own opinion: I don't like stupid getter (setter). there is no need to getEncoding(),.... change the scope from private to protected is fine and lighter.

Once again, sorry for taking so much time to reply, continue to contribute ! (and sorry for my rough english, it's not my mother tongue) Feel free to re-open or open and other PR if you have other contribution.