darkma773r commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547035454
##########
File path:
commons-geometry-core/src/test/java/org/apache/commons/geometry/core/partitioning/bsp/AbstractPartitionedRegionBuilderTest.java
##########
@@ -38,9 +38,7 @@ public void testCtor_invalidTree() {
final TestRegionBSPTree tree = new TestRegionBSPTree(true);
// act/assert
- GeometryTestUtils.assertThrowsWithMessage(() -> {
- new TestRegionBuilder(tree);
- }, IllegalArgumentException.class, "Tree must be empty");
+ GeometryTestUtils.assertThrowsWithMessage(() -> new
TestRegionBuilder(tree), IllegalArgumentException.class, "Tree must be empty");
Review comment:
I disagree with this change. The braces on the lambda are of course not
needed, but they were added for readability. With the original multi-line form,
the statement being tested is immediately visible. In the form proposed here,
the reader must scan the method arguments in order to determine what is
actually being tested. This is not so bad in this particular case, but in
others it becomes quite difficult to see what is happening. Please revert this
change and others like it.
##########
File path: pom.xml
##########
@@ -631,12 +631,12 @@
<developer>
<name>Gilles Sadowski</name>
<id>erans</id>
- <email>erans at apache dot org</email>
+ <email>[email protected]</email>
</developer>
<developer>
<name>Matt Juntunen</name>
<id>mattjuntunen</id>
- <email>mattjuntunen at apache dot org</email>
+ <email>[email protected]</email>
Review comment:
Please do not change the email formatting here or above. This becomes
part of the public website for the project (see
https://commons.apache.org/proper/commons-geometry/commons-geometry-core/team.html)
and it is up to the individual developer to decide how their email should be
displayed.
##########
File path:
commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/twod/rotation/Rotation2DTest.java
##########
@@ -150,20 +150,18 @@ public void testCreateRotationVector() {
final double max = 8;
final double step = 1;
- EuclideanTestUtils.permuteSkipZero(min, max, step, (ux, uy) -> {
- EuclideanTestUtils.permuteSkipZero(min, max, step, (vx, vy) -> {
+ EuclideanTestUtils.permuteSkipZero(min, max, step, (ux, uy) ->
EuclideanTestUtils.permuteSkipZero(min, max, step, (vx, vy) -> {
Review comment:
I do not see a need for this change. I find this new format harder to
read than the previous one since it obscures the fact that this is essentially
a nested for loop.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
