On Sep 19, 2018, at 9:58 AM, Remi Forax <forax@univ-mlv.fr> wrote:

Hi Jim, the signature of transform() in the webrev was not updated (so the wildcards are missing).

Apologies. I created the webrev before I fully saved. Will update in a bit.

And i'm still not convince this method should be introduced as is: - it need more variants (transformToInt, transformToLong, transformToDouble) to be useful, currently "foo".transform(String::length) do box the resulting int to an Integer. - pull it's own weight, while it's nice to be able to be able to write code fluently from left to right, "foo".transform(Utils::capitalizeFirstLetter), you can say exactly the same thing for all classes in the JDK, e.g. path.transform(Utils.appendTextSuffix). Other languages have introduced an operator to solve that issue (function call syntax is not fluent) like by example the operator '|>' as in "foo" |> Utils.capitalizeFirstLetter.

I hear you. Wouldn’t it be nice to have an Object::transform. Won't happen since it would likely break the world. You could push for something |> method(…) which applies static R method(T fakeThis, …), but that will take years of debate. String::transform was intended to facilitate custom manipulation (alignment) of raw string literals, in the most string generalized way. I’ll discuss the other variants but please provide better use cases. Cheers, — Jim

regards, Rémi

----- Mail original -----

...

De: "Jim Laskey" <james.laskey@oracle.com> À: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 18 Septembre 2018 19:52:17 Objet: RFR - JDK-8203442 String::transform (Code Review)

...

Please review the code for String::transform. The goal is to provide a String instance method to allow function application of custom transformations applied to an instance of String.

webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html jbs: https://bugs.openjdk.java.net/browse/JDK-8203442 csr: https://bugs.openjdk.java.net/browse/JDK-8203703

Cheers,

— Jim

updated webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html <http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html>

On Sep 21, 2018, at 7:42 AM, Remi Forax <forax@univ-mlv.fr> wrote:

----- Mail original -----

...

De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "Jim Laskey" <james.laskey@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 21 Septembre 2018 12:22:42 Objet: Re: RFR - JDK-8203442 String::transform (Code Review)

...

On 18/09/2018 18:52, Jim Laskey wrote:

...

Please review the code for String::transform. The goal is to provide a String instance method to allow function application of custom transformations applied to an instance of String.

webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html jbs: https://bugs.openjdk.java.net/browse/JDK-8203442 csr: https://bugs.openjdk.java.net/browse/JDK-8203703 I hate to bring up naming but if I have a Stream<String> or Optional<String> then I'll use the "map" method to apply the mapping function. Has this already been through the naming bike shed and it settled on "transform"? Maybe a different name was chosen after looking at code that would use it in stream operations? Maybe "transform" was used as the argument to the function is always text? I'm not interested in re-opening any discussions, just trying to see if options are captured in a mail somewhere.

I'm also wondering if this is general enough to be defined by CharSequence. Retrofitting CharSequence is always problematic and never done lightly but I'm just wondering if it has been discussed and dismissed. I don't think it could be done at a later time, at least not without changing the input type parameter so that the mapping function is Function<? super CharSequence, R> rather than Function<String, R>.

the main issue is that a lot of utility methods take a String as parameter, not a CharSequence :( and Function<? super String, R> is a supertype not a subtype of Function<? super CharSequence, R>.

...

-Alan

Rémi

I'm very uncomfortable about adding this method as is. Firstly, as Peter says (in a different thread), naming this method `map()` is inconsistent with `Stream` and `Optional`. Adding `map()` to `String` would be: public String map(Function<Character, Character>); Not that such a method would be particularly useful! Secondly, the premise of the method is effectively an alternative to the language feature "extension methods". While the motivation in https://bugs.openjdk.java.net/browse/JDK-8203703 and https://dzone.com/articles/making-java-fluent-api-more-flexible makes some sense, it is clearly equally applicable to all types - `String` is not special here. An isolated change here just looks odd and ill thought through - a big mistake for the core `String` class. My preference is for the change to not proceed without consideration of the broader context. The change should not be added without a commitment to also add the same method to Stream and Optional as a minimum, something that would make the addition part of a more consistent broader design. Doing this would also mandate a different method name. FWIW, were such a method added to java.time.* as the OP suggested, it would enable pluggable conversions: java.util.Date ju = LocalDate.now() .plusDays(2) .apply(MyUtility::convertToOldDate); Clearly there is some value to the above in terms of abstraction. However, there would also be problems making the best choice of types. Does it go on the concrete LocalDate or the interface Temporal. Its a problem just like String vs CharSequence. TLDR, this change needs to be part of a broader context to be acceptable (something that would inform the best name). Without that broader context, I'm against this change. Stephen On Mon, 12 Nov 2018 at 14:05, Jim Laskey <james.laskey@oracle.com> wrote:

updated webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html <http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html>

...

On Sep 21, 2018, at 7:42 AM, Remi Forax <forax@univ-mlv.fr> wrote:

----- Mail original -----

...

De: "Alan Bateman" <Alan.Bateman@oracle.com> À: "Jim Laskey" <james.laskey@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 21 Septembre 2018 12:22:42 Objet: Re: RFR - JDK-8203442 String::transform (Code Review)

...

On 18/09/2018 18:52, Jim Laskey wrote:

...

Please review the code for String::transform. The goal is to provide a String instance method to allow function application of custom transformations applied to an instance of String.

webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html jbs: https://bugs.openjdk.java.net/browse/JDK-8203442 csr: https://bugs.openjdk.java.net/browse/JDK-8203703 I hate to bring up naming but if I have a Stream<String> or Optional<String> then I'll use the "map" method to apply the mapping function. Has this already been through the naming bike shed and it settled on "transform"? Maybe a different name was chosen after looking at code that would use it in stream operations? Maybe "transform" was used as the argument to the function is always text? I'm not interested in re-opening any discussions, just trying to see if options are captured in a mail somewhere.

I'm also wondering if this is general enough to be defined by CharSequence. Retrofitting CharSequence is always problematic and never done lightly but I'm just wondering if it has been discussed and dismissed. I don't think it could be done at a later time, at least not without changing the input type parameter so that the mapping function is Function<? super CharSequence, R> rather than Function<String, R>.

the main issue is that a lot of utility methods take a String as parameter, not a CharSequence :( and Function<? super String, R> is a supertype not a subtype of Function<? super CharSequence, R>.

...

-Alan

Rémi

On 9/21/18 12:22 PM, Alan Bateman wrote:

On 18/09/2018 18:52, Jim Laskey wrote:

...

Please review the code for String::transform. The goal is to provide a String instance method to allow function application of custom transformations applied to an instance of String.

webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html jbs: https://bugs.openjdk.java.net/browse/JDK-8203442 csr: https://bugs.openjdk.java.net/browse/JDK-8203703 I hate to bring up naming but if I have a Stream<String> or Optional<String> then I'll use the "map" method to apply the mapping function.

An argument against re-using the name map() for this String method is that Stream.map() and Optional.map() act on the element(s) of the "container" the method is invoked upon, and return the same raw part of type with type parameter adjusted, while String.map() would be passing the target object itself to the function and returning an arbitrary type. So in this regard, it is a different operation. The same method as suggested for String would be usable on Stream too, but it would have to be called differently on Stream. Imagine defining this method on Object. It would clash with Stream.map() and Optional.map() if it was called map(). So I don't think .map() is the best name for this method. Regards, Peter

Hi Brian, ----- Mail original -----

De: "Brian Goetz" <brian.goetz@oracle.com> À: "Peter Levart" <peter.levart@gmail.com> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Mardi 13 Novembre 2018 15:37:31 Objet: Re: RFR - JDK-8203442 String::transform (Code Review)

...

An argument against re-using the name map() for this String method is that Stream.map() and Optional.map() act on the element(s) of the "container" the method is invoked upon, and return the same raw part of type with type parameter adjusted, while String.map() would be passing the target object itself to the function and returning an arbitrary type.

This is exactly why we did not choose the name map() when this feature was proposed. (Such is life when a platform is accrete-only; you have to live with your past decisions, good and bad.)

(I didn’t see the proposal to rename transform() to map() fly by; I would have made the same comments as Peter.)

as i said earlier, we have https://bugs.openjdk.java.net/browse/JDK-8140283 that asks for the same kind of method on Stream, i think it's a good idea to have some kind or coordination here. While 'transform' is a ok name for String, it's a bad name for Stream. All intermediary ops are transformations, but 'transform' doesn't subsume them. A method named transform will act as a magnet for Stream newbies that want to do transformation on a stream. Perhaps 'chain' as proposed in JDK-8140283 is a better name ?

...

Other languages have introduced an operator to solve that issue (function call syntax is not fluent) like by example the operator '|>' as in "foo" |> Utils.capitalizeFirstLetter.

Yes, we know :) But we don’t have any current plans to do that, nor use-site extension methods, nor does it seem likely to come to the top of the language priority list very soon. So its not a choice between |> and .transform(); it’s a choice between transform() and nothing. Picking transform() seems pretty good here.

Stephen raised the issue of a “broader context”; indeed, the intention of adding a transform() method here was that it would be feasible to add to other types for which it was suitable. String is an obvious candidate for “do it first”, but this is within a broader context.

ok about the rationale, we just need to find a good name. Rémi

On Tue, 13 Nov 2018 at 15:44, Brian Goetz <brian.goetz@oracle.com> wrote:

Yes, we know :) But we don’t have any current plans to do that, nor use-site extension methods, nor does it seem likely to come to the top of the language priority list very soon. So its not a choice between |> and .transform(); it’s a choice between transform() and nothing. Picking transform() seems pretty good here.

Stephen raised the issue of a “broader context”; indeed, the intention of adding a transform() method here was that it would be feasible to add to other types for which it was suitable. String is an obvious candidate for “do it first”, but this is within a broader context.

I'd be more comforted if there was a commitment to add the method to Stream and Optional in Java 12 or 13. I also agree with Remi that `transform()` is a poor name for Stream, and thus it is a poor name for String. I doubt there is a perfect name, process() or apply() seem like good candidates. Stephen

I fully agree with Stephen. Rémi ----- Mail original -----

De: "Stephen Colebourne" <scolebourne@joda.org> À: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 30 Novembre 2018 12:06:23 Objet: Re: RFR - JDK-8203442 String::transform (Code Review)

I see from Twitter (!!!) that this has been pushed. This appears to have happened without this thread coming to a clear conclusion, particularly wrt criticism of transform() as a suitable method name in the broader context.

I also do not think that the code review was completed correctly and in public, which I find concerning.

The two public threads are: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.ht... http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.ht...

The last webrev had the name map() and no further webrev was published that I can see. I can see no explicit public approvals of the change from reviewers. And plenty of concern about the method name, especially map() but also transform().

While I'm well aware of the danger of public bikeshedding, I also think that adding methods to `String` deserves more care than this has been shown. In my view the change should be reverted, and retargetted to Java 13 to allow for a proper conclusion to the discussion.

For info, AWS SDK v2 has chosen applyMutation() for a similar concept: https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/utils/build...

Stephen

On Wed, 14 Nov 2018, 14:18 Stephen Colebourne <scolebourne@joda.org wrote:

...

On Tue, 13 Nov 2018 at 15:44, Brian Goetz <brian.goetz@oracle.com> wrote:

...

Yes, we know :) But we don’t have any current plans to do that, nor use-site extension methods, nor does it seem likely to come to the top of the language priority list very soon. So its not a choice between |> and .transform(); it’s a choice between transform() and nothing. Picking transform() seems pretty good here.

Stephen raised the issue of a “broader context”; indeed, the intention of adding a transform() method here was that it would be feasible to add to other types for which it was suitable. String is an obvious candidate for “do it first”, but this is within a broader context.

I'd be more comforted if there was a commitment to add the method to Stream and Optional in Java 12 or 13.

I also agree with Remi that `transform()` is a poor name for Stream, and thus it is a poor name for String. I doubt there is a perfect name, process() or apply() seem like good candidates.

Stephen

Hi, My 2 cents: 1) I agree that `String.map` is a really unfortunate name - Peter explained it very well! I'd rather wait than have `String.map`. 2) I absolutely agree that `String`, `Stream`, and `Optional` should share the name. 3) I prefer `Stream.transform` to `Stream.chain` but I understand it might be confusing to newbies. BTW. Stream-like `transform` has already been used by Lukas Eder in his `Seq`: https://www.jooq.org/products/jOO λ/javadoc/0.9.14/org/jooq/lambda/Seq.html#transform-java.util.function.Function- 4) I'm not convinced by `asInputTo` (too many words) nor by `applyMutation` (confusing, like Tagir said). On a side note: regardless of the name, I bet we'll see some `string.transform(String::toLowerCase)` or `stream.chain(s->s.map(mapper))` :) Regards, Tomasz Linkowski On Mon, Dec 3, 2018 at 12:32 PM Tagir Valeev <amaembo@gmail.com> wrote:

+1 to Stephen and Remi. I personally see no reason to hurry up with this API: unlike trimming/indenting methods this one doesn't look crucial for raw string literals. I don't see that this method blocks anything else.

...

For info, AWS SDK v2 has chosen applyMutation() for a similar concept:

Unfortunately this name serves well only for mutable objects, like builders. For String it looks confusing.

With best regards, Tagir Valeev. On Sat, Dec 1, 2018 at 5:27 AM Remi Forax <forax@univ-mlv.fr> wrote:

...

I fully agree with Stephen.

Rémi

----- Mail original -----

...

De: "Stephen Colebourne" <scolebourne@joda.org> À: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 30 Novembre 2018 12:06:23 Objet: Re: RFR - JDK-8203442 String::transform (Code Review)

...

I see from Twitter (!!!) that this has been pushed. This appears to

have

...

...

happened without this thread coming to a clear conclusion, particularly wrt criticism of transform() as a suitable method name in the broader context.

I also do not think that the code review was completed correctly and in public, which I find concerning.

The two public threads are:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.ht...

...

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.ht...

...

The last webrev had the name map() and no further webrev was published

that

...

I can see. I can see no explicit public approvals of the change from reviewers. And plenty of concern about the method name, especially map() but also transform().

While I'm well aware of the danger of public bikeshedding, I also think that adding methods to `String` deserves more care than this has been shown. In my view the change should be reverted, and retargetted to Java 13 to allow for a proper conclusion to the discussion.

For info, AWS SDK v2 has chosen applyMutation() for a similar concept:

https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/utils/build...

...

Stephen

On Wed, 14 Nov 2018, 14:18 Stephen Colebourne <scolebourne@joda.org

wrote:

...

...

On Tue, 13 Nov 2018 at 15:44, Brian Goetz <brian.goetz@oracle.com>

wrote:

...

...

...

Yes, we know :) But we don’t have any current plans to do that, nor use-site extension methods, nor does it seem likely to come to the top of the language priority list very soon. So its not a choice between |> and .transform(); it’s a choice between transform() and nothing. Picking transform() seems pretty good here.

Stephen raised the issue of a “broader context”; indeed, the intention of adding a transform() method here was that it would be feasible to add to other types for which it was suitable. String is an obvious candidate for “do it first”, but this is within a broader context.

I'd be more comforted if there was a commitment to add the method to Stream and Optional in Java 12 or 13.

I also agree with Remi that `transform()` is a poor name for Stream, and thus it is a poor name for String. I doubt there is a perfect name, process() or apply() seem like good candidates.

Stephen

-- Pozdrawiam, Tomasz Linkowski

Thank you for following up - we all know the core-libs team has a busy workload, and naming topics are always tricky. I'm personally unconvinced that `transform()` is the best name out there. While transform is OK for String and maybe Optional, it is poor for List and Stream. In addition, I'm still not overly convinced that this is an appropriate stylistic direction for Java to go more generally (though I fully agree with point #2 on language change below). However, since the core-libs team is clearly indicating a desire to close the topic, I have no personal intention of objecting further.. thanks Stephen On Tue, 4 Dec 2018 at 18:38, Stuart Marks <stuart.marks@oracle.com> wrote:

Hi everybody,

I've finally caught up with all of this. I see that several people are surprised by the way this has turned out. As the first-named reviewer on this changeset, I felt I should try to figure out what happened.

While this API point stands on its own, this is really part of Jim's RSL work [1] which includes several API additions to String, and which will likely have a significant effect on how String literals are used in Java code.

Jim started code review [2] and CSR review [3] for this proposal back in September. There was a gap of several weeks, followed by a revised proposal on 12 Nov [4][5] that changed the method's name from "transform" to "map".

There was further discussion over the next couple days; in particular there were some objections to the method name "map". On 26 Nov Jim pushed the changeset with the name changed back to "transform".

From reading just the messages on the mailing list, I can certainly see why people were surprised. It looked like there was an issue open for discussion, yet Jim went ahead and pushed the revised change anyway. What happened?

Given that the primary open issue was about naming, and such mailing list discussions can go on for an arbitrarily long time, Jim asked Brian off-list for a decision on this. The results were:

1) the name for this method on String should be "transform";

2) adding library methods is a reasonable way for the platform to provide this capability (as opposed to various language enhancements that might be contemplated); and

3) the intent is to colonize the name "transform" for this operation on this and other classes, such as List, Optional, etc. (It may well be the case that there is no name that is a good fit for both Stream and for everything else, given Stream's highly stylized API; we can consider the tradeoffs between consistency and clarity when we get to that particular one.)

The primary thing that went wrong here was that Brian and Jim neglected to circle back to the list to record this decision. This was an oversight.

There could and should have been better communication about this. Brian, Jim, or I should have documented the results of this decision and followed up on the mailing list. None of us did. Sorry about that.

What else should be done here?

One thing is that we (I) can make a better effort to keep up on emails, though the volume -- on a wide variety of topics -- is significant.

Another point is that issues that are raised on the mailing list are often discussed and resolved off the mailing list. While we make significant and ongoing efforts to write up relevant offline discussions for public consumption (see, for example, see the recent writeup on nullable values [6]), sometimes things will fall through the cracks.

Finally, we should also record that this particular decision sets a precedent for the use of the name "transform" for methods that do similar things on other classes. To this end, I've updated this bug with a note about "transform":

JDK-8140283 [7] add method to Stream that facilitates fluent chaining of other methods

and I've also filed the following RFE:

JDK-8214753 [8] (opt) add Optional::transform, allowing an arbitrary operation on an Optional

s'marks

[1] http://openjdk.java.net/jeps/326

[2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055532.h...

[3] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055533.h...

[4] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.ht...

[5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.ht...

[6]

http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2018-November/0...

[7] https://bugs.openjdk.java.net/browse/JDK-8140283

[8] https://bugs.openjdk.java.net/browse/JDK-8214753