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 15:43:01 UTC
[GitHub] [commons-geometry] singhbaljit opened a new pull request #122: Miscellaneous (cherry-picked) improvements
singhbaljit opened a new pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#discussion_r547373970
##########
File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Bounds3D.java
##########
@@ -233,7 +233,7 @@ public Builder add(final Vector3D pt) {
* @param pts points to add
* @return this instance
*/
- public Builder addAll(final Iterable<Vector3D> pts) {
+ public Builder addAll(final Iterable<? extends Vector3D> pts) {
Review comment:
Users can't create their own `Vector3D` subclasses since the constructor is private. I don't see a reason to prevent use of `Iterable<Vector3D.Unit>`, though, so I think we should go ahead with this change.
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#discussion_r547331628
##########
File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Bounds3D.java
##########
@@ -233,7 +233,7 @@ public Builder add(final Vector3D pt) {
* @param pts points to add
* @return this instance
*/
- public Builder addAll(final Iterable<Vector3D> pts) {
+ public Builder addAll(final Iterable<? extends Vector3D> pts) {
Review comment:
Although there is one subclass within the library, users can define other subclasses of `Vector3D` (for whatever reason).
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#discussion_r547374247
##########
File path: commons-geometry-core/src/main/java/org/apache/commons/geometry/core/partitioning/AbstractConvexHyperplaneBoundedRegion.java
##########
@@ -161,7 +161,7 @@ public String toString() {
*/
protected <R extends AbstractConvexHyperplaneBoundedRegion<P, S>> R transformInternal(
final Transform<P> transform, final R thisInstance, final Class<S> boundaryType,
- final Function<List<S>, R> factory) {
+ final Function<? super List<S>, R> factory) {
Review comment:
Okey dokey.
----------------------------------------------------------------
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 edited a comment on pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
singhbaljit edited a comment on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-748216059
@arturobernalg I kept them separate so its easy to remove/rebase a specific change.
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
darkma773r commented on a change in pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#discussion_r547040251
##########
File path: commons-geometry-core/src/main/java/org/apache/commons/geometry/core/partitioning/AbstractConvexHyperplaneBoundedRegion.java
##########
@@ -161,7 +161,7 @@ public String toString() {
*/
protected <R extends AbstractConvexHyperplaneBoundedRegion<P, S>> R transformInternal(
final Transform<P> transform, final R thisInstance, final Class<S> boundaryType,
- final Function<List<S>, R> factory) {
+ final Function<? super List<S>, R> factory) {
Review comment:
I see the reasoning behind this but I'm not sure if it's needed since tt makes an already crazy method signature even crazier. WDYT? This goes for the other `super` generic changes as well.
##########
File path: commons-geometry-core/src/test/java/org/apache/commons/geometry/core/GeometryTestUtils.java
##########
@@ -120,7 +121,7 @@ public static Object serializeAndRecover(final Object obj) {
final ByteArrayInputStream bis = new ByteArrayInputStream(bos.toByteArray());
final ObjectInputStream si = new ObjectInputStream(bis);
return si.readObject();
- } catch (final Exception e) {
+ } catch (final IOException | ClassNotFoundException e) {
Review comment:
We actually don't use this method at all so we could just delete it.
##########
File path: commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Bounds3D.java
##########
@@ -233,7 +233,7 @@ public Builder add(final Vector3D pt) {
* @param pts points to add
* @return this instance
*/
- public Builder addAll(final Iterable<Vector3D> pts) {
+ public Builder addAll(final Iterable<? extends Vector3D> pts) {
Review comment:
Is this worth it? The only subclass of `Vector3D` is `Vector3D.Unit`.
----------------------------------------------------------------
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 edited a comment on pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
arturobernalg edited a comment on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-748197996
LGTM. Maybe would be good idea made a squash? @singhbaljit WDYT
----------------------------------------------------------------
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 edited a comment on pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
arturobernalg edited a comment on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-748197996
LGTM. Maybe would be good idea made a squash? WDYT
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#discussion_r547330636
##########
File path: commons-geometry-core/src/main/java/org/apache/commons/geometry/core/partitioning/AbstractConvexHyperplaneBoundedRegion.java
##########
@@ -161,7 +161,7 @@ public String toString() {
*/
protected <R extends AbstractConvexHyperplaneBoundedRegion<P, S>> R transformInternal(
final Transform<P> transform, final R thisInstance, final Class<S> boundaryType,
- final Function<List<S>, R> factory) {
+ final Function<? super List<S>, R> factory) {
Review comment:
IMO, it improves the usability of the APIs. See Josh Bloch's _Effective Java_ (3rd Edition):
> Using wildcard types in your APIs, while tricky, makes the APIs far more flexible. If you write a library that will be widely used, the proper use of wildcard types should be considered mandatory.
Actually, some of the generic parameters can also wildcards (i.e. `final Class<? extends S> boundaryType`), but I kept those out for now to avoid overcomplicating the signature even more.
----------------------------------------------------------------
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 pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
darkma773r commented on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-751462301
@singhbaljit, can you rebase and fix conflicts here? We can then merge this in.
----------------------------------------------------------------
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 pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-748216059
@arturobernalg I kept them separate so its remove/rebase a specific change.
----------------------------------------------------------------
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 pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
darkma773r commented on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-755029147
Oops. Guess I missed that.
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-748197996
LGTM
----------------------------------------------------------------
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 pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-748221671
Will wait for #123 to avoid merge conflicts/duplicate work.
----------------------------------------------------------------
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 #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on a change in pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#discussion_r547330636
##########
File path: commons-geometry-core/src/main/java/org/apache/commons/geometry/core/partitioning/AbstractConvexHyperplaneBoundedRegion.java
##########
@@ -161,7 +161,7 @@ public String toString() {
*/
protected <R extends AbstractConvexHyperplaneBoundedRegion<P, S>> R transformInternal(
final Transform<P> transform, final R thisInstance, final Class<S> boundaryType,
- final Function<List<S>, R> factory) {
+ final Function<? super List<S>, R> factory) {
Review comment:
IMO, it improves the usability of the APIs. See Josh Bloch's _Effective Java_ (3rd Edition):
> Using wildcard types in your APIs, while tricky, makes the APIs far more flexible. If you write a library that will be widely used, the proper use of wildcard types should be considered mandatory.
Actually, some of the generic parameters can also use wildcards (i.e. `final Class<? extends S> boundaryType`), but I kept those out for now to avoid overcomplicating the signature even more.
----------------------------------------------------------------
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 pull request #122: Miscellaneous (cherry-picked) improvements
Posted by GitBox <gi...@apache.org>.
singhbaljit commented on pull request #122:
URL: https://github.com/apache/commons-geometry/pull/122#issuecomment-755014852
@darkma773r its been rebased.
----------------------------------------------------------------
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