Skip to content

Conversation

@nosan
Copy link
Contributor

@nosan nosan commented Jul 20, 2018

add a new method PropertyMapper.from(value).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 20, 2018
@nosan nosan force-pushed the polish-property-mapper branch from 2d78148 to 2e6d26a Compare July 23, 2018 10:29
@philwebb
Copy link
Member

Thanks for the PR. I actually considered adding this method at the time but I was nervous about providing an overloaded version of a lambda method. I've seen a few people suggest that it's not good practice to do so.

Perhaps we use a different method name instead? Maybe fromValue(...)?

@nosan
Copy link
Contributor Author

nosan commented Jul 23, 2018

@philwebb Yes, you are right, but this could happen only for explicit null value.

Thoughts?

Thanks.

@philwebb
Copy link
Member

I'm at the limit of the knowledge on the subject and I can't remember who told me about it. I certainly like the feel of of not needing a different method name. I'll flag for team attention and see if there's a general consensus.

@philwebb
Copy link
Member

@rstoyanchev @smaldini Was it either of you two that mentioned about not overloading lambda calls? Perhaps during some of the early reactor work?

@nosan nosan force-pushed the polish-property-mapper branch 2 times, most recently from e54fd8a to 7f7e4fd Compare July 24, 2018 12:34
@simonbasle
Copy link

simonbasle commented Aug 16, 2018

@philwebb I can remember that we actively avoided something slightly different in Reactor (that was a focus of the 3.1 release): avoid to provide several signatures that all take a functional interface with same number of parameters, eg. from(Supplier) vs from(Callable), because these would become ambiguous when using lambdas.

It is also better for integration into languages like Kotlin (cc @sdeleuze) to avoid functional interface overloads at all (as Kotlin has optional parameters, which brings back the ambiguity even with Consumer<T> vs BiConsumer<T, R>).

I don't think from(T) vs from(Supplier<T>) falls into this category, and thus should be fine 😉

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Aug 16, 2018
@nosan nosan force-pushed the polish-property-mapper branch from 7f7e4fd to 893a052 Compare August 19, 2018 15:35
@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Aug 22, 2018
@snicoll snicoll self-assigned this Aug 22, 2018
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 22, 2018
@snicoll snicoll added this to the 2.1.0.M3 milestone Aug 22, 2018
snicoll pushed a commit that referenced this pull request Aug 22, 2018
@snicoll snicoll closed this in 597fe23 Aug 22, 2018
snicoll added a commit that referenced this pull request Aug 22, 2018
* pr/13837:
  Polish "Add PropertyMapper.from(value)"
  Add PropertyMapper.from(value)
@nosan nosan deleted the polish-property-mapper branch August 22, 2018 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants