That was fast.
On 16/01/2017 22:29, Michael Osipov wrote:
Here are my comments referred in my the previous mail:
* README.txt requires a rewrite, it still talks about Ant, old
directory structures, and much more
Ok.
* 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.
* 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.
* There are several spots which
** don't use SLF4J's {} placeholder,
** don't use System.lineSeparator() but '\n'
** do a toLowerCase() (e.g., #sameEncoding()) instead of
toLowerCase(Locale.ROOT)
* There is a lot trailing whitespace throughout the code
* Generally, don't use log#isXXXEnabled() because of SLF4J's {}
placeholders' foo
I'll check those ones.
* There are a few null argument checks which can be better handled
with Common Lang's Validate class
Where, and why?
* StringBuffer is used sometimes where StringBuilder would suffice
(w/o synchronization)
Ok.
* 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?).
* 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.
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.
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org
