Am 2017-01-16 um 22:56 schrieb Claude Brisson:
That was fast.
On 16/01/2017 22:29, Michael Osipov wrote:
Here are my comments referred in my the previous mail:
* POM: use of JavaCC Maven Plugin: never ever add auto-generated code
to src/, it completely breaks
the agreed convention. Have it in target/ and attach the source code
with Buildhelper Maven Plugin, if
the JavaCC Plugin doesn't do it on its own.
I totally agree. I left things as I found them basically because I had
enough other things to learn about Maven by the time. If you want to
help further on this one you are welcome.
I will have a look, especially at the patching of the generated source
code. I assume that this is absolutely necessary.
* POM: The deletion/creation of the JavaCC parser should rather be a
complete profile rather than
a hack with properties passed to Ant, if this is necessary at all. I
see no difference here to code generation by JAXB's XJC or Modello, etc.
We could also regererate it each time. It's not that painful.
Absolutely, this is cheap. At best, the plugin would have a stale
detection based on mtime, but this is out of our business.
* There are a few null argument checks which can be better handled with Common
Lang's Validate class
Where, and why?
See:
$ grep -r "throw new IllegalAr" .
./velocity-engine-core/src/main/java/org/apache/velocity/io/VelocityWriter.java:
throw new IllegalArgumentException("Buffer size <= 0");
./velocity-engine-core/src/main/java/org/apache/velocity/runtime/directive/RuntimeMacro.java:
throw new IllegalArgumentException("Null arguments");
./velocity-engine-core/src/main/java/org/apache/velocity/util/ArrayIterator.java:
throw new IllegalArgumentException(
./velocity-engine-core/src/main/java/org/apache/velocity/util/ExtProperties.java:
throw new IllegalArgumentException('\'' + token + "' does not contain " + "an equals
sign");
./velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/IntrospectorBase.java:
throw new IllegalArgumentException ("class object is null!");
./velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/IntrospectorBase.java:
throw new IllegalArgumentException("params object is null!");
./velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/IntrospectorBase.java:
throw new IllegalArgumentException("class object is null!");
./velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/IntrospectorCache.java:
throw new IllegalArgumentException("class is null!");
./velocity-engine-core/src/main/java/org/apache/velocity/util/introspection/IntrospectorCache.java:
throw new IllegalArgumentException("class is null!");
It basically does all you need, null with NPE, invalid values with IAE,
states with ISE according to today's Java exception conventions. Plus,
less brevity and boilerplate code.
Sample commit:
https://github.com/apache/maven/commit/618e62dd3315b4cb5b0f7dcdd37fc787c44b2ade
* Stuff like BUILD_README.txt absolutely do not belong to
src/main/java, see second point
* There are some unused imports once in a while
* Rather use maven.compiler.source and maven.compiler.target
properties to avoid duplicate value setting, checkstyle, findbugs,
JavaCC, etc.
You mean between different pom files? I don't see several occurrences of
findbugs, javacc, etc... and none of checkstyle (should there be any?).
Yes, between different ones. See m-compiler-p in parent and
javacc-maven-plugin core. If you add PMD too, you'll get three spots for
updating.
* If a dependency is used several times, e.g. SFL4J, it shall be added
to the dep mngt section of the parent POM to avoid repetition of
version elements. Hint: this can be done with reactor modules too
It's version has been factorized in the parent.
This isn't what was referring to. You still have repetitive clutter like
<version>${....version>}</version> whereas the better way is to like
here:
https://git-wip-us.apache.org/repos/asf?p=maven-wagon.git;a=blob;f=pom.xml;h=006fbe9bcf505e46e26a9e3906966262547e1e2b;hb=HEAD#l284
Modules only provide groupId and artifactId, the rest in inherited.
There are likely to be more issues I have missed.
What is the benefit of velocity-engine-assembly actually? I know, ages
ago Velocity and other software was delivered as a tarball or zip file
with all JARs and the rest, including source. I think this is pretty
much obsolete now and partially redundant:
* Apache Parent already provides predefined descriptors which create a
source-release.zip for you, no further work necessary
** source-release.zip is used for release testing/votes, as well as
for clients/users who do not want to clone from Git or checkout via
Subversion
* Binary artifacts are always available on Maven Central, I hardly
believe that someone will really download dist.zip to get the JARs and
add them manually somewhere, even if, they can get them from
search.maven.org
I'm +1 on this one. Let's drop assembly.
+1
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org
