Fix for 5015163, and my first webrev
Hi all, bug 5015163 is a RFE that advocate to add a method join (like in Python) to String/StringBuilder/StringBuffer. Here is the corresponding webrev (using the new infrastucture :) http://cr.openjdk.java.net/~forax/5015163/webrev.00/ The patch is against tl repository. Who want to review it ? For the record, i have previously proposed this fix using the jdk-collaboration forum more than one year ago but because i am lazy i haven't take the time to post it here. cheers, Rémi Forax
public String join(Object first, Object... elements) { if (elements.length==0) return String.valueOf(first); return new StringBuilder().join(this, first, elements).toString(); } It does not look right to simply return String.valueOf(first); when elements size is 0, where is "this"? No, I'm not endorsing the APIs provided, at least for now:-) there is room to debate what would be the best choice(APIs) to support the "joint", if we decided to add one. Sherman Rémi Forax wrote:
Hi all, bug 5015163 is a RFE that advocate to add a method join (like in Python) to String/StringBuilder/StringBuffer.
Here is the corresponding webrev (using the new infrastucture :) http://cr.openjdk.java.net/~forax/5015163/webrev.00/ The patch is against tl repository.
Who want to review it ?
For the record, i have previously proposed this fix using the jdk-collaboration forum more than one year ago but because i am lazy i haven't take the time to post it here.
cheers, Rémi Forax
Xueming Shen a écrit :
public String join(Object first, Object... elements) { if (elements.length==0) return String.valueOf(first); return new StringBuilder().join(this, first, elements).toString(); }
It does not look right to simply return String.valueOf(first); when elements size is 0, where is "this"? "this" is the delimiter.
",".join("hello", "world") => "hello,world" Thus it doesn't use "this" if there is only one argument.
No, I'm not endorsing the APIs provided, at least for now:-) there is room to debate what would be the best choice(APIs) to support the "joint", if we decided to add one.
Is it more clear now ? By the way, I've written 3 tests, one by class (String, StringBuilder, StringBuffer) Should I add them to the webrev ?
Sherman
Rémi
Rémi Forax wrote:
Xueming Shen a écrit :
public String join(Object first, Object... elements) { if (elements.length==0) return String.valueOf(first); return new StringBuilder().join(this, first, elements).toString(); }
It does not look right to simply return String.valueOf(first); when elements size is 0, where is "this"? "this" is the delimiter.
",".join("hello", "world") => "hello,world"
Thus it doesn't use "this" if there is only one argument. oops, did not pay attention to the "deimiter part:-)
Hello, A few thoughts. First, this functionality is badly needed. Absolutely everyone rewrites this, in hundreds of different ways. At Google we're no exception, we have our own hand-rolled Join.java class full of static methods, and it has thousands of callers in our private codebase. Your selection of overloads (Iterable, Object[], and (Object,Object...)) is exactly correct. Kudos. We'd added Iterator versions as well, but almost no one uses them, so forget that. My main concerns with your current API are: * I don't think that ",".join(collection) is a good idea. It clashes too strongly with the StringBuilder etc. versions. It also plain "looks weird". A static String.join(",", collection) is preferable. I don't think anyone would be surprised by this. * It should be considered separately whether to support other Appendables via static methods (somewhere) like: static <A extends Appendable> A join(A appendable, String delimiter, Iterable<?> tokens) throws IOException This provides a superset of what your StringBuilder etc. methods can do, but your methods are still good because of their convenience especially in chains of method calls, and because they don't need to throw any exception. * The biggest problem we've had with our Join class is that it turns out that everyone has a different idea of how they'd like nulls handled. - output as "null" (your, and our, current behavior) - skip? - skip only if trailing? - throw NPE? (often the best thing) We now support everyone by having the concept of an instantiable Joiner (I've seen this in other languages). Example: private static final Joiner JOINER = Joiner.with(", ").skipNulls(); . . . return JOINER.join("Harry", "Ron", "Hermione"); Just to give everyone food for thought, our Joiner's API currently looks like this: static Joiner with(String separator) Joiner skipNulls() Joiner useForNull(String nullText) <A extends Appendable> A appendTo(A appendable, Iterable<?> parts) throws IOException <A extends Appendable> A appendTo(A appendable, Object[] parts) throws IOException <A extends Appendable> A appendTo(A appendable, Object first, Object... rest) throws IOException StringBuilder appendTo(StringBuilder builder, Iterable<?> parts) StringBuilder appendTo(StringBuilder builder, Object[] parts) StringBuilder appendTo(StringBuilder builder, Object first, Object... rest) String join(Iterable<?> parts) String join(Object[] parts) String join(Object first, Object... rest) MapJoiner withKeyValueSeparator(String keyValueSeparator) class MapJoiner { <A extends Appendable> A appendTo(A appendable, Map<?, ?> map) throws IOException StringBuilder appendTo(StringBuilder builder, Map<?, ?> map) String join(Map<?, ?> map) MapJoiner useForNull(String nullText) } End of random thoughts. On Fri, Feb 6, 2009 at 12:59 PM, Rémi Forax <forax@univ-mlv.fr> wrote:
Hi all, bug 5015163 is a RFE that advocate to add a method join (like in Python) to String/StringBuilder/StringBuffer.
Here is the corresponding webrev (using the new infrastucture :) http://cr.openjdk.java.net/~forax/5015163/webrev.00/ The patch is against tl repository.
Who want to review it ?
For the record, i have previously proposed this fix using the jdk-collaboration forum more than one year ago but because i am lazy i haven't take the time to post it here.
cheers, Rémi Forax
-- Kevin Bourrillion @ Google internal: http://go/javalibraries google-collections.googlecode.com google-guice.googlecode.com
Kevin Bourrillion a écrit :
Hello,
A few thoughts.
First, this functionality is badly needed. Absolutely everyone rewrites this, in hundreds of different ways. At Google we're no exception, we have our own hand-rolled Join.java class full of static methods, and it has thousands of callers in our private codebase.
Your selection of overloads (Iterable, Object[], and (Object,Object...)) is exactly correct. Kudos. We'd added Iterator versions as well, but almost no one uses them, so forget that.
My main concerns with your current API are:
* I don't think that ",".join(collection) is a good idea. It clashes too strongly with the StringBuilder etc. versions. It also plain "looks weird". A static String.join(",", collection) is preferable. I don't think anyone would be surprised by this.
Use the receiver or not: Pro: - join works with split and split use the receiver - Python use the receiver too Cons: - static seems least surprising. - Perl's join and PHP's join are functions I'm convinced, I will modify the patch.
* It should be considered separately whether to support other Appendables via static methods (somewhere) like:
static <A extends Appendable> A join(A appendable, String delimiter, Iterable<?> tokens) throws IOException
This provides a superset of what your StringBuilder etc. methods can do, but your methods are still good because of their convenience especially in chains of method calls, and because they don't need to throw any exception.
Or perhaps, in JDK8 (when closure will be integrated) the compiler will be able to infer that because join is used with String, it can't throw an IOE That's not what BGGA propose but I will love to see that :) But that's another story.
* The biggest problem we've had with our Join class is that it turns out that everyone has a different idea of how they'd like nulls handled.
- output as "null" (your, and our, current behavior) - skip? - skip only if trailing? - throw NPE? (often the best thing)
In my opinion (i am perhaps wong) join will be mostly used just before print the String. That's why I have chosen the the print(ln) behavior.
We now support everyone by having the concept of an instantiable Joiner (I've seen this in other languages). Example:
private static final Joiner JOINER = Joiner.with(", ").skipNulls(); . . .
return JOINER.join("Harry", "Ron", "Hermione");
Just to give everyone food for thought, our Joiner's API currently looks like this:
static Joiner with(String separator)
Joiner skipNulls() Joiner useForNull(String nullText)
<A extends Appendable> A appendTo(A appendable, Iterable<?> parts) throws IOException <A extends Appendable> A appendTo(A appendable, Object[] parts) throws IOException <A extends Appendable> A appendTo(A appendable, Object first, Object... rest) throws IOException
StringBuilder appendTo(StringBuilder builder, Iterable<?> parts) StringBuilder appendTo(StringBuilder builder, Object[] parts) StringBuilder appendTo(StringBuilder builder, Object first, Object... rest)
String join(Iterable<?> parts) String join(Object[] parts) String join(Object first, Object... rest)
MapJoiner withKeyValueSeparator(String keyValueSeparator)
class MapJoiner { <A extends Appendable> A appendTo(A appendable, Map<?, ?> map) throws IOException StringBuilder appendTo(StringBuilder builder, Map<?, ?> map) String join(Map<?, ?> map)
MapJoiner useForNull(String nullText) }
End of random thoughts.
Cool, i don't think this should be integrated in the JDK, After all, If i want a full featured joiner, I can download the google collection jar :)
-- Kevin Bourrillion @ Google internal: http://go/javalibraries google-collections.googlecode.com google-guice.googlecode.com
Rémi
Bumping this thread. From the peanut gallery, I believe that Rémi's change is extremely worthy. I have more nitpicking I'd like to do on it, but am holding off until we learn whether Sun would accept it in any shape. At this point, it's still very mysterious to me how this will work (small, simple new APIs being added to core libraries via openjdk). On Fri, Feb 6, 2009 at 5:01 PM, Rémi Forax <forax@univ-mlv.fr> wrote:
Kevin Bourrillion a écrit :
Hello,
A few thoughts.
First, this functionality is badly needed. Absolutely everyone rewrites this, in hundreds of different ways. At Google we're no exception, we have our own hand-rolled Join.java class full of static methods, and it has thousands of callers in our private codebase.
Your selection of overloads (Iterable, Object[], and (Object,Object...)) is exactly correct. Kudos. We'd added Iterator versions as well, but almost no one uses them, so forget that.
My main concerns with your current API are:
* I don't think that ",".join(collection) is a good idea. It clashes too strongly with the StringBuilder etc. versions. It also plain "looks weird". A static String.join(",", collection) is preferable. I don't think anyone would be surprised by this.
Use the receiver or not: Pro: - join works with split and split use the receiver - Python use the receiver too
Cons: - static seems least surprising. - Perl's join and PHP's join are functions
I'm convinced, I will modify the patch.
* It should be considered separately whether to support other
Appendables via static methods (somewhere) like:
static <A extends Appendable> A join(A appendable, String delimiter, Iterable<?> tokens) throws IOException
This provides a superset of what your StringBuilder etc. methods can do, but your methods are still good because of their convenience especially in chains of method calls, and because they don't need to throw any exception.
Or perhaps, in JDK8 (when closure will be integrated) the compiler will be able to infer that because join is used with String, it can't throw an IOE That's not what BGGA propose but I will love to see that :) But that's another story.
* The biggest problem we've had with our Join class is that it turns out that everyone has a different idea of how they'd like nulls handled.
- output as "null" (your, and our, current behavior) - skip? - skip only if trailing? - throw NPE? (often the best thing)
In my opinion (i am perhaps wong) join will be mostly used just before print the String. That's why I have chosen the the print(ln) behavior.
We now support everyone by having the concept of an instantiable
Joiner (I've seen this in other languages). Example:
private static final Joiner JOINER = Joiner.with(", ").skipNulls(); . . .
return JOINER.join("Harry", "Ron", "Hermione");
Just to give everyone food for thought, our Joiner's API currently looks like this:
static Joiner with(String separator)
Joiner skipNulls() Joiner useForNull(String nullText)
<A extends Appendable> A appendTo(A appendable, Iterable<?> parts) throws IOException <A extends Appendable> A appendTo(A appendable, Object[] parts) throws IOException <A extends Appendable> A appendTo(A appendable, Object first, Object... rest) throws IOException
StringBuilder appendTo(StringBuilder builder, Iterable<?> parts) StringBuilder appendTo(StringBuilder builder, Object[] parts) StringBuilder appendTo(StringBuilder builder, Object first, Object... rest)
String join(Iterable<?> parts) String join(Object[] parts) String join(Object first, Object... rest)
MapJoiner withKeyValueSeparator(String keyValueSeparator)
class MapJoiner { <A extends Appendable> A appendTo(A appendable, Map<?, ?> map) throws IOException StringBuilder appendTo(StringBuilder builder, Map<?, ?> map) String join(Map<?, ?> map)
MapJoiner useForNull(String nullText) }
End of random thoughts.
Cool, i don't think this should be integrated in the JDK, After all, If i want a full featured joiner, I can download the google collection jar :)
-- Kevin Bourrillion @ Google internal: http://go/javalibraries google-collections.googlecode.com google-guice.googlecode.com
Rémi
-- Kevin Bourrillion @ Google internal: http://go/javalibraries google-collections.googlecode.com google-guice.googlecode.com
Kevin Bourrillion wrote:
Bumping this thread. From the peanut gallery, I believe that Rémi's change is extremely worthy. I have more nitpicking I'd like to do on it, but am holding off until we learn whether Sun would accept it in any shape.
At this point, it's still very mysterious to me how this will work (small, simple new APIs being added to core libraries via openjdk). I agree it's worth continuing this. As you said, it's one of those things that almost everyone has written many times.
Mark sent out a link a few days ago to a draft process for jdk7 development [1]. I assume in time this will detail the process for reviewing API additions and replacing the current (Sun-internal) CCC process. -Alan. [1] http://mail.openjdk.java.net/pipermail/jdk7-dev/2009-February/000411.html
Rémi, can you tell me how you got the cr.openjdk.java.net account ? Is there any registration link? Thanks, Ulf Am 06.02.2009 21:59, Rémi Forax schrieb:
Hi all, bug 5015163 is a RFE that advocate to add a method join (like in Python) to String/StringBuilder/StringBuffer.
Here is the corresponding webrev (using the new infrastucture :) http://cr.openjdk.java.net/~forax/5015163/webrev.00/ The patch is against tl repository.
Who want to review it ?
For the record, i have previously proposed this fix using the jdk-collaboration forum more than one year ago but because i am lazy i haven't take the time to post it here.
cheers, Rémi Forax
Le 14/10/2009 16:47, Ulf Zibis a écrit :
Rémi,
can you tell me how you got the cr.openjdk.java.net account ? Is there any registration link?
Thanks,
Ulf
I've got it because I've the right to push codes on mlvm repository (the DaVinci machine repository). I can use the same openjdk account to push webrevs on cr.openjdk.java.net. Rémi PS: now, I feel guilty because you remember me that I have to work on a patch to mlvm repository for John Rose .
Am 06.02.2009 21:59, Rémi Forax schrieb:
Hi all, bug 5015163 is a RFE that advocate to add a method join (like in Python) to String/StringBuilder/StringBuffer.
Here is the corresponding webrev (using the new infrastucture :) http://cr.openjdk.java.net/~forax/5015163/webrev.00/ The patch is against tl repository.
Who want to review it ?
For the record, i have previously proposed this fix using the jdk-collaboration forum more than one year ago but because i am lazy i haven't take the time to post it here.
cheers, Rémi Forax
Am 14.10.2009 17:01, Rémi Forax schrieb:
Le 14/10/2009 16:47, Ulf Zibis a écrit :
Rémi,
can you tell me how you got the cr.openjdk.java.net account ? Is there any registration link?
Thanks,
Ulf
I've got it because I've the right to push codes on mlvm repository (the DaVinci machine repository). I can use the same openjdk account to push webrevs on cr.openjdk.java.net.
Ah, ok, so I'm wondering why you are asking for sponsorship if you have commit rights. -Ulf
2009/10/14 Ulf Zibis <Ulf.Zibis@gmx.de>:
Am 14.10.2009 17:01, Rémi Forax schrieb:
Le 14/10/2009 16:47, Ulf Zibis a écrit :
Rémi,
can you tell me how you got the cr.openjdk.java.net account ? Is there any registration link?
Thanks,
Ulf
I've got it because I've the right to push codes on mlvm repository (the DaVinci machine repository). I can use the same openjdk account to push webrevs on cr.openjdk.java.net.
Ah, ok, so I'm wondering why you are asking for sponsorship if you have commit rights.
-Ulf
Ulf,
From what I saw, he was asking for a review and approval of the patch rather than sponsorship. Patches still need to be approved even if the person has commit rights (even if this is not so clear from the number of Sun commits that are approved behind the scenes). -- Andrew :-)
Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Support Free Java! Contribute to GNU Classpath and the OpenJDK http://www.gnu.org/software/classpath http://openjdk.java.net PGP Key: 94EFD9D8 (http://subkeys.pgp.net) Fingerprint: F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8
Am 14.10.2009 23:19, Andrew John Hughes schrieb:
2009/10/14 Ulf Zibis <Ulf.Zibis@gmx.de>:
Am 14.10.2009 17:01, Rémi Forax schrieb:
Le 14/10/2009 16:47, Ulf Zibis a écrit :
Rémi,
can you tell me how you got the cr.openjdk.java.net account ? Is there any registration link?
Thanks,
Ulf
I've got it because I've the right to push codes on mlvm repository (the DaVinci machine repository). I can use the same openjdk account to push webrevs on cr.openjdk.java.net.
Ah, ok, so I'm wondering why you are asking for sponsorship if you have commit rights.
-Ulf
Ulf,
From what I saw, he was asking for a review and approval of the patch rather than sponsorship. Patches still need to be approved even if the person has commit rights (even if this is not so clear from the number of Sun commits that are approved behind the scenes).
Andrew, sorry for the confusion. My question regarding sponsorship referred to http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-October/002967.htm... Thanks for your answer anyway! -Ulf
participants (6)
-
Alan Bateman
-
Andrew John Hughes
-
Kevin Bourrillion
-
Rémi Forax
-
Ulf Zibis
-
Xueming Shen