Re: RFR - JDK-8203442 String::transform (Code Review)

[Tomasz Linkowski]

Alan,  Rémi,

If you're still interested in providing transform-like behavior for
CharSequence, there's a fairly neat (I hope) way to do that.

The idea is to use the following helper functional interface for forwarding
to the already added String.transform:


@FunctionalInterface
interface Transformer {
   R by(Function f);
}


public interface CharSequence extends Transformable {
  // ...
  Transformer transformed();
  // ...
}


public final class String implements /*...*/ CharSequence {
  // ...
  @Override
  public Transformer transformed() {
return this::transform;
  }
  // ...
}


Usage:

CharSequence transformed = charSeq
.transformed().by(s -> StringUtils.defaultIfBlank('0')) // sample
Function
.transformed().by(...)
.transformed().by(...);


If you find it interesting, it's described in detail in my blog post:
https://blog.tlinkowski.pl/2018/transformer-pattern/

Regards,
Tomasz Linkowski


On Fri, Sep 21, 2018 at 12:42 PM Remi Forax  wrote:

> - Mail original -
> > De: "Alan Bateman" 
> > À: "Jim Laskey" , "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 or
> > Optional 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 rather than Function.
>
> the main issue is that a lot of utility methods take a String as
> parameter, not a CharSequence :(
> and Function is a supertype not a subtype of Function super CharSequence, R>.
>
> >
> > -Alan
>
> Rémi
>


-- 
Pozdrawiam,
Tomasz Linkowski

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stephen Colebourne]

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  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.html
>
> [3]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055533.html
>
> [4]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.html
>
> [5]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html
>
> [6]
>
> http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2018-November/000784.html
>
> [7] https://bugs.openjdk.java.net/browse/JDK-8140283
>
> [8] https://bugs.openjdk.java.net/browse/JDK-8214753
>
>

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stuart Marks]

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

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

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

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

[6] 
http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2018-November/000784.html


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

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

Re: RFR - JDK-8203442 String::transform (Code Review)

[Tomasz Linkowski]

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  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  wrote:
> >
> > I fully agree with Stephen.
> >
> > Rémi
> >
> > - Mail original -
> > > De: "Stephen Colebourne" 
> > > À: "core-libs-dev" 
> > > 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.html
> > >
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html
> > >
> > > 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/builder/SdkBuilder.html#applyMutation-java.util.function.Consumer-
> > >
> > > Stephen
> > >
> > >
> > > On Wed, 14 Nov 2018, 14:18 Stephen Colebourne  wrote:
> > >
> > >> On Tue, 13 Nov 2018 at 15:44, Brian Goetz 
> 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

Re: RFR - JDK-8203442 String::transform (Code Review)

[Tagir Valeev]

+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  wrote:
>
> I fully agree with Stephen.
>
> Rémi
>
> - Mail original -
> > De: "Stephen Colebourne" 
> > À: "core-libs-dev" 
> > 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.html
> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html
> >
> > 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/builder/SdkBuilder.html#applyMutation-java.util.function.Consumer-
> >
> > Stephen
> >
> >
> > On Wed, 14 Nov 2018, 14:18 Stephen Colebourne  >
> >> On Tue, 13 Nov 2018 at 15:44, Brian Goetz  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

Re: RFR - JDK-8203442 String::transform (Code Review)

[Remi Forax]

I fully agree with Stephen.

Rémi

- Mail original -
> De: "Stephen Colebourne" 
> À: "core-libs-dev" 
> 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.html
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html
> 
> 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/builder/SdkBuilder.html#applyMutation-java.util.function.Consumer-
> 
> Stephen
> 
> 
> On Wed, 14 Nov 2018, 14:18 Stephen Colebourne  
>> On Tue, 13 Nov 2018 at 15:44, Brian Goetz  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

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stephen Colebourne]

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.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html

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/builder/SdkBuilder.html#applyMutation-java.util.function.Consumer-

Stephen


On Wed, 14 Nov 2018, 14:18 Stephen Colebourne  On Tue, 13 Nov 2018 at 15:44, Brian Goetz  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
>

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stephen Colebourne]

On Tue, 13 Nov 2018 at 15:44, Brian Goetz  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

RE: RFR - JDK-8203442 String::transform (Code Review)

[Anthony Vanelverdinghe]

Hi Remi, Brian,



What about ‘asInputTo’ or some variant thereof (e.g. using Argument or the 
abbreviated Arg instead of Input; or dropping/replacing either of the 
prefix/suffix)?



This would give, for example:

`raw html`.asInputTo(HtmlDocument::new)

source.stream().asInputTo(this::maybeAddFilter)



To me, this name clearly relates the object with its argument: the object 
serves ‘as input to’ the argument (plus it hints at the fact that the argument 
is a Function). And since it’s just using general terminology, it’s applicable 
in a broad context, yet is unlikely to clash or look awkward anywhere.



Kind regards,

Anthony






From: core-libs-dev  on behalf of Remi 
Forax 
Sent: Wednesday, November 14, 2018 12:36:16 PM
To: Brian Goetz
Cc: core-libs-dev
Subject: Re: RFR - JDK-8203442 String::transform (Code Review)

Hi Brian,

- Mail original -
> De: "Brian Goetz" 
> À: "Peter Levart" 
> Cc: "core-libs-dev" 
> 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

Re: RFR - JDK-8203442 String::transform (Code Review)

[Remi Forax]

Hi Brian,

- Mail original -
> De: "Brian Goetz" 
> À: "Peter Levart" 
> Cc: "core-libs-dev" 
> 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

Re: RFR - JDK-8203442 String::transform (Code Review)

[Brian Goetz]

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

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

Re: RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Stephen Colebourne]

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);
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  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  wrote:
> >
> > - Mail original -
> >> De: "Alan Bateman" 
> >> À: "Jim Laskey" , "core-libs-dev" 
> >> 
> >> 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 or
> >> Optional 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 rather than Function.
> >
> > the main issue is that a lot of utility methods take a String as parameter, 
> > not a CharSequence :(
> > and Function is a supertype not a subtype of Function > super CharSequence, R>.
> >
> >>
> >> -Alan
> >
> > Rémi
>

Re: RFR - JDK-8203442 String::transform (Code Review)

[Peter Levart]

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

Re: RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Andrej Golovnin]

Hi Jim,

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

src/java.base/share/classes/java/lang/String.java

2983  * @param   class of the result

Maybe "the type of the result" would be better.

Best regards,
Andrej Golovnin

RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Jim Laskey]

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  wrote:
> 
> - Mail original -
>> De: "Alan Bateman" 
>> À: "Jim Laskey" , "core-libs-dev" 
>> 
>> 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 or
>> Optional 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 rather than Function.
> 
> the main issue is that a lot of utility methods take a String as parameter, 
> not a CharSequence :(
> and Function is a supertype not a subtype of Function super CharSequence, R>.
> 
>> 
>> -Alan
> 
> Rémi

Re: RFR - JDK-8203442 String::transform (Code Review)

[Remi Forax]

- Mail original -
> De: "Alan Bateman" 
> À: "Jim Laskey" , "core-libs-dev" 
> 
> 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 or
> Optional 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 rather than Function.

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

> 
> -Alan

Rémi

Re: RFR - JDK-8203442 String::transform (Code Review)

[Alan Bateman]

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 or 
Optional 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 rather than Function.


-Alan

Re: RFR - JDK-8203442 String::transform (Code Review)

[Jim Laskey]

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

> On Sep 19, 2018, at 10:35 AM, Jim Laskey  wrote:
> 
>> On Sep 19, 2018, at 9:58 AM, Remi Forax  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" 
>>> À: "core-libs-dev" 
>>> 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
> 

Re: RFR - JDK-8203442 String::transform (Code Review)

[Jim Laskey]

> On Sep 19, 2018, at 9:58 AM, Remi Forax  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" 
>> À: "core-libs-dev" 
>> 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

Re: RFR - JDK-8203442 String::transform (Code Review)

[Remi Forax]

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

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.

regards,
Rémi

- Mail original -
> De: "Jim Laskey" 
> À: "core-libs-dev" 
> 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

RFR - JDK-8203442 String::transform (Code Review)

[Jim Laskey]

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