Re: [VOTE] Engine 2.0 RC5 Release quality

[Claude Brisson]

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.



Maybe not. I'm playing with it. Looks easier than I thought.



-
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org

Re: [VOTE] Engine 2.0 RC5 Release quality

[Claude Brisson]

Changing my vote to

[X] Leave at test build

after Michael remarks (sigh...).

On 16/01/2017 15:04, Claude Brisson wrote:

And my vote:


 [ ] Leave at test build
 [ ] Alpha
 [ ] Beta
 [X] General Availability (GA)



  Claude


-
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

Re: [VOTE] Engine 2.0 RC5 Release quality

[Michael Osipov]

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>} 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, 

Re: [VOTE] Engine 2.0 RC5 Release quality

[Claude Brisson]

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

Re: [VOTE] Engine 2.0 RC5 Release quality

[Michael Osipov]

Am 2017-01-16 um 15:02 schrieb Claude Brisson:

The Velocity Engine 2.0 RC5 is available.

Main changes since the RC4:

 * the default encoding is now UTF-8 (and not the platform default)
 * commons-collections is not any more a compilation dependency
 * commons-lang3 dependency is not any more shaded
 * the configuration API doesn't use ExtProperties anymore
 * the events API has been reviewed: all events do receive the current
Context as argument

Release notes:

*
https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.0/release-notes.html


Distribution:

 * https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.0/

Maven 2 staging repository:

 *
https://repository.apache.org/content/repositories/orgapachevelocity-1015

If you have had a chance to review the test build, please respond with a
vote on its quality:

 [ ] Leave at test build
 [ ] Alpha
 [ ] Beta
 [ ] General Availability (GA)


Everyone who has tested the build is invited to vote. Votes by PMC
members are considered binding. A vote passes if there are at least
three binding +1s and more +1s than -1s.


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
* 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.
* 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.

* 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
* There are a few null argument checks which can be better handled with 
Common Lang's Validate class
* StringBuffer is used sometimes where StringBuilder would suffice (w/o 
synchronization)
* 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.
* 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


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



Michael

-
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org

Re: [VOTE] Engine 2.0 RC5 Release quality

[Michael Osipov]

Am 2017-01-16 um 15:02 schrieb Claude Brisson:

The Velocity Engine 2.0 RC5 is available.

Main changes since the RC4:

 * the default encoding is now UTF-8 (and not the platform default)
 * commons-collections is not any more a compilation dependency
 * commons-lang3 dependency is not any more shaded
 * the configuration API doesn't use ExtProperties anymore
 * the events API has been reviewed: all events do receive the current
Context as argument

Release notes:

*
https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.0/release-notes.html


Distribution:

 * https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.0/

Maven 2 staging repository:

 *
https://repository.apache.org/content/repositories/orgapachevelocity-1015

If you have had a chance to review the test build, please respond with a
vote on its quality:

 [ ] Leave at test build
 [ ] Alpha
 [ ] Beta
 [ ] General Availability (GA)


Everyone who has tested the build is invited to vote. Votes by PMC
members are considered binding. A vote passes if there are at least
three binding +1s and more +1s than -1s.


I have started a review based on a diff between 1.7 and trunk. I will 
report shortly. There are already some issues with the POM which should 
be improved.


Michael


-
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org

Re: [VOTE] Engine 2.0 RC5 Release quality

[Greg Huber]

[ ] Leave at test build
[ ] Alpha
[ ] Beta
[x] General Availability (GA)

+1 works great for me.  Thanks.

On 16 January 2017 at 14:02, Claude Brisson  wrote:

> The Velocity Engine 2.0 RC5 is available.
>
> Main changes since the RC4:
>
>  * the default encoding is now UTF-8 (and not the platform default)
>  * commons-collections is not any more a compilation dependency
>  * commons-lang3 dependency is not any more shaded
>  * the configuration API doesn't use ExtProperties anymore
>  * the events API has been reviewed: all events do receive the current
> Context as argument
>
> Release notes:
>
> * https://dist.apache.org/repos/dist/dev/velocity/velocity-eng
> ine/2.0/release-notes.html
>
> Distribution:
>
>  * https://dist.apache.org/repos/dist/dev/velocity/velocity-engine/2.0/
>
> Maven 2 staging repository:
>
>  * https://repository.apache.org/content/repositories/orgapache
> velocity-1015
>
> If you have had a chance to review the test build, please respond with a
> vote on its quality:
>
>  [ ] Leave at test build
>  [ ] Alpha
>  [ ] Beta
>  [ ] General Availability (GA)
>
>
> Everyone who has tested the build is invited to vote. Votes by PMC members
> are considered binding. A vote passes if there are at least three binding
> +1s and more +1s than -1s.
>
>
>
>   Claude
>
>
>

Re: [VOTE] Engine 2.0 RC5 Release quality

[Claude Brisson]

And my vote:


 [ ] Leave at test build
 [ ] Alpha
 [ ] Beta
 [X] General Availability (GA)



  Claude


-
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org