You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/05/19 14:45:40 UTC

[GitHub] [zookeeper] belugabehr opened a new pull request #1359: ZOOKEEPER-3837: Deprecate StringUtils Join

belugabehr opened a new pull request #1359:
URL: https://github.com/apache/zookeeper/pull/1359


   


----------------------------------------------------------------
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] [zookeeper] nkalmar commented on pull request #1359: ZOOKEEPER-3837: Deprecate StringUtils Join

Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1359:
URL: https://github.com/apache/zookeeper/pull/1359#issuecomment-631623171


   Oh, you got that error because you don't supress the warning, or even better, just fix it up there also!


----------------------------------------------------------------
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] [zookeeper] belugabehr commented on pull request #1359: ZOOKEEPER-3837: Deprecate StringUtils Join

Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1359:
URL: https://github.com/apache/zookeeper/pull/1359#issuecomment-632095051


   > I would also deprecate the split to be honest. I'm not fan of implementing something so general ourselves, especially that java already is pretty good ad string operations, but we also have apache.io. Didn't dig into it, but probably there is some one liner for a split similar.
   
   Ya, I looked into this, and I can, but I didn't because it's not exactly the same since it trims each result.


----------------------------------------------------------------
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] [zookeeper] belugabehr commented on a change in pull request #1359: ZOOKEEPER-3837: Deprecate StringUtils Functions

Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1359:
URL: https://github.com/apache/zookeeper/pull/1359#discussion_r428674068



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/StringUtils.java
##########
@@ -48,6 +48,12 @@ private StringUtils() {/** non instantiable and non inheritable **/}
      * into a single string, where the original strings are separated using
      * the given delimiter.
      *
+     * Deprecated. Use {@link String#join(CharSequence, CharSequence...)}

Review comment:
       > Any build warnings can be suppressed by updating any calling code to suppress the warning
   
   Yup, I was missing this piece.  Thanks!




----------------------------------------------------------------
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] [zookeeper] belugabehr commented on pull request #1359: ZOOKEEPER-3837: Deprecate StringUtils Join

Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1359:
URL: https://github.com/apache/zookeeper/pull/1359#issuecomment-630883360


   > [WARNING] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/admin/ZooKeeperAdmin.java:[319,24] joinStrings(java.util.List<java.lang.String>,java.lang.String) in org.apache.zookeeper.common.StringUtils has been deprecated
   [INFO] 6 warnings 
   [INFO] -------------------------------------------------------------
   [INFO] -------------------------------------------------------------
   [ERROR] COMPILATION ERROR : 
   [INFO] -------------------------------------------------------------
   [ERROR] /home/travis/build/apache/zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/admin/ZooKeeperAdmin.java: warnings found and -Werror specified
   [INFO] 1 error
   
   That's unfortunate that I can't deprecate things.


----------------------------------------------------------------
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] [zookeeper] nkalmar commented on pull request #1359: ZOOKEEPER-3837: Deprecate StringUtils Join

Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1359:
URL: https://github.com/apache/zookeeper/pull/1359#issuecomment-631624327


   I would also deprecate the split to be honest. I'm not fan of implementing something so general ourselves, especially that java already is pretty good ad string operations, but we also have apache.io. Didn't dig into it, but probably there is some one liner for a split similar.


----------------------------------------------------------------
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] [zookeeper] ctubbsii commented on a change in pull request #1359: ZOOKEEPER-3837: Deprecate StringUtils Join

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1359:
URL: https://github.com/apache/zookeeper/pull/1359#discussion_r428645551



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/StringUtils.java
##########
@@ -48,6 +48,12 @@ private StringUtils() {/** non instantiable and non inheritable **/}
      * into a single string, where the original strings are separated using
      * the given delimiter.
      *
+     * Deprecated. Use {@link String#join(CharSequence, CharSequence...)}

Review comment:
       This should be a proper javadoc deprecated tag, something liike:
   
   ```java
   /**
     * @deprecated since ZK version ... Use {@link ....} instead.`
     */
   ```
   
   And, the method should have a `@Deprecated` Java annotation. Any build warnings can be suppressed by updating any calling code to suppress the warning or use its replacement.




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