Zookeeper 2814: ignore space after comma in connection string #345
Conversation
Class which provides common String utilities
|
|
||
| import org.apache.zookeeper.common.PathUtils; | ||
| import static org.apache.zookeeper.common.StringUtils.split; | ||
|
|
There was a problem hiding this comment.
afine: Removed it. please review
| import java.util.List; | ||
|
|
||
| import org.apache.zookeeper.common.PathUtils; | ||
| import static org.apache.zookeeper.common.StringUtils.split; |
There was a problem hiding this comment.
nit: I could be wrong on this, but I think in general we prefer to import classes and not static methods directly.
There was a problem hiding this comment.
Static import of individual member is allowed as it sometimes improves the readability
Please let me know your opinion on this and if required I will change it.
There was a problem hiding this comment.
This code is similar to existing one in master and branch-3.5. For consistency, we could use what kind of import it uses, imo.
There was a problem hiding this comment.
eribeiro: I have kept the code same, in master and branch-3.5 static method member is imported i.e import static org.apache.zookeeper.common.StringUtils.split
There was a problem hiding this comment.
eribeiro: Can you please complete the review comments? Can you please approve the changes so that patch can be created.
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.zookeeper.common; |
There was a problem hiding this comment.
afine: added required space. please review
There was a problem hiding this comment.
Hi afine: Can you please complete the review comments? Can you please approve the changes so that patch can be created.
| public void testStrings() { | ||
|
|
||
| String s1 = " a , b , "; | ||
| assertEquals("[a, b]", StringUtils.split(s1, ",").toString()); |
There was a problem hiding this comment.
I'm wondering if it would be possible to compare lists themselves rather than comparing the output of toString?
There was a problem hiding this comment.
afine: Conversion to string is required as output of StringUtils.split returns java.util.Collections$UnmodifiableRandomAccessList<[a, b]>
There was a problem hiding this comment.
What is wrong with using
String s1 = " a , b , ";
List<String> expected = Arrays.asList("a", "b");
assertTrue(expected.equals(StringUtils.split(s1, ",")));
????
There was a problem hiding this comment.
It works, and I can change the asserts. However, I have retrofitted the test from master and branch 3.5 so is it okay to change it in this particular branch? Please let me know.
nikhilbhide
left a comment
nikhilbhide
left a comment
There was a problem hiding this comment.
I have made required changes. Please review
| import java.util.List; | ||
|
|
||
| import org.apache.zookeeper.common.PathUtils; | ||
| import static org.apache.zookeeper.common.StringUtils.split; |
There was a problem hiding this comment.
Static import of individual member is allowed as it sometimes improves the readability
Please let me know your opinion on this and if required I will change it.
|
|
||
| import org.apache.zookeeper.common.PathUtils; | ||
| import static org.apache.zookeeper.common.StringUtils.split; | ||
|
|
There was a problem hiding this comment.
afine: Removed it. please review
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.zookeeper.common; |
There was a problem hiding this comment.
afine: added required space. please review
| public void testStrings() { | ||
|
|
||
| String s1 = " a , b , "; | ||
| assertEquals("[a, b]", StringUtils.split(s1, ",").toString()); |
There was a problem hiding this comment.
afine: Conversion to string is required as output of StringUtils.split returns java.util.Collections$UnmodifiableRandomAccessList<[a, b]>
nikhilbhide
left a comment
nikhilbhide
left a comment
There was a problem hiding this comment.
Please review and revert
| public void testStrings() { | ||
|
|
||
| String s1 = " a , b , "; | ||
| assertEquals("[a, b]", StringUtils.split(s1, ",").toString()); |
There was a problem hiding this comment.
It works, and I can change the asserts. However, I have retrofitted the test from master and branch 3.5 so is it okay to change it in this particular branch? Please let me know.
nikhilbhide
left a comment
nikhilbhide
left a comment
There was a problem hiding this comment.
Please review
nikhilbhide
left a comment
nikhilbhide
left a comment
There was a problem hiding this comment.
eribeiro & afine: Can you please complete the review comments? Can you please approve the changes so that patch can be created.
|
Patch is ready to be applied |
|
Patch is ready |
nikhilbhide
deleted the
ZOOKEEPER-2814-Ignore-space-after-comma-in-connection-string
branch
3 participants
No description provided.