You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/12/18 17:17:16 UTC
[GitHub] [commons-geometry] arturobernalg opened a new pull request #123: Minor Improvement
arturobernalg opened a new pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123
* 'for' loop replaceable with enhanced 'for' loop
* Unnecessary unboxing
* Explicit type can be replaced with <>
* Statement lambda can be replaced with expression lambda
* Call to 'Arrays.asList()' with too few arguments
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547810950
##########
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:
done
----------------------------------------------------------------
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
[GitHub] [commons-geometry] asfgit merged pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123
----------------------------------------------------------------
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
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547379511
##########
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:
This comment applies to all updates of `GeometryTestUtils.assertThrowsWithMessage()`, not just this instance.
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547103361
##########
File path: commons-geometry-examples/examples-io/src/test/java/org/apache/commons/geometry/examples/io/threed/obj/OBJWriterTest.java
##########
@@ -65,7 +65,6 @@ public void testClose_calledMultipleTimes() throws IOException {
// act/assert
try (OBJWriter meshWriter = new OBJWriter(writer)) {
meshWriter.close();
- meshWriter.close();
Review comment:
:)
----------------------------------------------------------------
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
[GitHub] [commons-geometry] singhbaljit commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r546739823
##########
File path: commons-geometry-examples/examples-io/src/test/java/org/apache/commons/geometry/examples/io/threed/obj/OBJWriterTest.java
##########
@@ -65,7 +65,6 @@ public void testClose_calledMultipleTimes() throws IOException {
// act/assert
try (OBJWriter meshWriter = new OBJWriter(writer)) {
meshWriter.close();
- meshWriter.close();
Review comment:
Also, what is the need to call `close()` for _try-with-resources_?
----------------------------------------------------------------
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
[GitHub] [commons-geometry] singhbaljit commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r546735653
##########
File path: commons-geometry-examples/examples-io/src/test/java/org/apache/commons/geometry/examples/io/threed/obj/OBJWriterTest.java
##########
@@ -65,7 +65,6 @@ public void testClose_calledMultipleTimes() throws IOException {
// act/assert
try (OBJWriter meshWriter = new OBJWriter(writer)) {
meshWriter.close();
- meshWriter.close();
Review comment:
@darkma773r is this a bug or intentionally calling `close()` thrice? Is twice sufficient?
----------------------------------------------------------------
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
[GitHub] [commons-geometry] singhbaljit commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r546735653
##########
File path: commons-geometry-examples/examples-io/src/test/java/org/apache/commons/geometry/examples/io/threed/obj/OBJWriterTest.java
##########
@@ -65,7 +65,6 @@ public void testClose_calledMultipleTimes() throws IOException {
// act/assert
try (OBJWriter meshWriter = new OBJWriter(writer)) {
meshWriter.close();
- meshWriter.close();
Review comment:
@darkma773r is this a bug or intentionally calling `close()` twice?
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r546789841
##########
File path: commons-geometry-examples/examples-io/src/test/java/org/apache/commons/geometry/examples/io/threed/obj/OBJWriterTest.java
##########
@@ -65,7 +65,6 @@ public void testClose_calledMultipleTimes() throws IOException {
// act/assert
try (OBJWriter meshWriter = new OBJWriter(writer)) {
meshWriter.close();
- meshWriter.close();
Review comment:
No idea.
----------------------------------------------------------------
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
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547039302
##########
File path: commons-geometry-examples/examples-io/src/test/java/org/apache/commons/geometry/examples/io/threed/obj/OBJWriterTest.java
##########
@@ -65,7 +65,6 @@ public void testClose_calledMultipleTimes() throws IOException {
// act/assert
try (OBJWriter meshWriter = new OBJWriter(writer)) {
meshWriter.close();
- meshWriter.close();
Review comment:
Twice is fine. I was just making _extra_ sure that we hit that `if` condition in `close()` :-)
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#issuecomment-749381885
Hi @darkma773r
I've revert all changes proposed. I really didn't know about the @ in the mail
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#issuecomment-748857492
ping @singhbaljit
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547384785
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -100,28 +96,9 @@ public static void assertSimpleEqualsCases(final Object obj) {
Assertions.assertFalse(obj.equals(new Object()), "Object should not equal an instance of different type");
}
- Assertions.assertTrue(obj.equals(obj), "Object should equal itself");
- }
+ Assertions.assertTrue(obj.
- /**
- * Serializes and then recovers an object from a byte array. Returns the deserialized object.
- *
- * @param obj object to serialize and recover
- * @return the recovered, deserialized object
- */
- public static Object serializeAndRecover(final Object obj) {
- try {
- // serialize the Object
- final ByteArrayOutputStream bos = new ByteArrayOutputStream();
- final ObjectOutputStream so = new ObjectOutputStream(bos);
- so.writeObject(obj);
- // deserialize the Object
- final ByteArrayInputStream bis = new ByteArrayInputStream(bos.toByteArray());
- final ObjectInputStream si = new ObjectInputStream(bis);
- return si.readObject();
- } catch (final Exception e) {
- throw new RuntimeException(e);
- }
+ equals(obj), "Object should equal itself");
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [commons-geometry] singhbaljit commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r545989644
##########
File path: commons-geometry-examples/examples-io/src/main/java/org/apache/commons/geometry/examples/io/threed/DefaultModelIOHandlerRegistry.java
##########
@@ -28,8 +29,8 @@
/** Construct a new instance and register known handlers.
*/
public DefaultModelIOHandlerRegistry() {
- setHandlers(Arrays.asList(
+ setHandlers(Collections.singletonList(
Review comment:
Make it one line?
##########
File path: commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/mesh/SimpleTriangleMeshTest.java
##########
@@ -261,8 +261,8 @@ public void testFaces_iterator() {
Vector3D.of(0, 1, 0)
);
- final List<int[]> faceIndices = Arrays.asList(
- new int[] {0, 1, 2}
+ final List<int[]> faceIndices = Collections.singletonList(
Review comment:
Make it one line?
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r545992155
##########
File path: commons-geometry-euclidean/src/test/java/org/apache/commons/geometry/euclidean/threed/mesh/SimpleTriangleMeshTest.java
##########
@@ -261,8 +261,8 @@ public void testFaces_iterator() {
Vector3D.of(0, 1, 0)
);
- final List<int[]> faceIndices = Arrays.asList(
- new int[] {0, 1, 2}
+ final List<int[]> faceIndices = Collections.singletonList(
Review comment:
why not. Changed
##########
File path: commons-geometry-examples/examples-io/src/main/java/org/apache/commons/geometry/examples/io/threed/DefaultModelIOHandlerRegistry.java
##########
@@ -28,8 +29,8 @@
/** Construct a new instance and register known handlers.
*/
public DefaultModelIOHandlerRegistry() {
- setHandlers(Arrays.asList(
+ setHandlers(Collections.singletonList(
Review comment:
Changed
----------------------------------------------------------------
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
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #123: Minor Improvement
Posted by 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
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547378485
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -100,28 +96,9 @@ public static void assertSimpleEqualsCases(final Object obj) {
Assertions.assertFalse(obj.equals(new Object()), "Object should not equal an instance of different type");
}
- Assertions.assertTrue(obj.equals(obj), "Object should equal itself");
- }
+ Assertions.assertTrue(obj.
- /**
- * Serializes and then recovers an object from a byte array. Returns the deserialized object.
- *
- * @param obj object to serialize and recover
- * @return the recovered, deserialized object
- */
- public static Object serializeAndRecover(final Object obj) {
- try {
- // serialize the Object
- final ByteArrayOutputStream bos = new ByteArrayOutputStream();
- final ObjectOutputStream so = new ObjectOutputStream(bos);
- so.writeObject(obj);
- // deserialize the Object
- final ByteArrayInputStream bis = new ByteArrayInputStream(bos.toByteArray());
- final ObjectInputStream si = new ObjectInputStream(bis);
- return si.readObject();
- } catch (final Exception e) {
- throw new RuntimeException(e);
- }
+ equals(obj), "Object should equal itself");
Review comment:
Remove blank lines.
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547103104
##########
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:
OK. reverted
##########
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:
reverted
----------------------------------------------------------------
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
[GitHub] [commons-geometry] singhbaljit commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r546735653
##########
File path: commons-geometry-examples/examples-io/src/test/java/org/apache/commons/geometry/examples/io/threed/obj/OBJWriterTest.java
##########
@@ -65,7 +65,6 @@ public void testClose_calledMultipleTimes() throws IOException {
// act/assert
try (OBJWriter meshWriter = new OBJWriter(writer)) {
meshWriter.close();
- meshWriter.close();
Review comment:
@darkma773r is this a bug or intentionally calling `close()` thrice?
----------------------------------------------------------------
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
[GitHub] [commons-geometry] arturobernalg commented on a change in pull request #123: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #123:
URL: https://github.com/apache/commons-geometry/pull/123#discussion_r547103023
##########
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:
done
----------------------------------------------------------------
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