[GitHub] [commons-geometry] darkma773r commented on a change in pull request #123: Minor Improvement

[GitBox <gi...@apache.org>]

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>erans@apache.org</email>
     </developer>
     <developer>
       <name>Matt Juntunen</name>
       <id>mattjuntunen</id>
-      <email>mattjuntunen at apache dot org</email>
+      <email>mattjuntunen@apache.org</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:
users@infra.apache.org