Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support lambda based converters by parsing bean method signature generics #22885

Open
wants to merge 1 commit into
base: 2.7.x
Choose a base branch
from

Conversation

viviel
Copy link

@viviel viviel commented Aug 11, 2020

If I define a Converter with lambda or method reference, the project will fail to start due to the failure of parsing generics. I don't think that's reasonable, so after understanding the reasons, I modified it to support some of the above features

@Bean
public Converter<String, Integer> myConverter1() {
	return Integer::valueOf;
}
@Bean
public Converter<String, Integer> myConverter2() {
	return e -> Integer.valueOf(e);
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 11, 2020
@wilkinsona
Copy link
Member

Thanks for the pull request, @viviel. Could you please add some tests that illustrate the problem that you changes solve?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Aug 11, 2020
@viviel
Copy link
Author

viviel commented Aug 11, 2020

OK, then I'll look at the CI problem

@wilkinsona
Copy link
Member

wilkinsona commented Aug 11, 2020

Thank you. Please don't worry too much about the CI failure. There's a known problem with the latest Spring Data snapshots that the Spring Data team are working on at the moment. A quick look at the test failures suggests that many, if not all of them, are related to that.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 11, 2020
@viviel
Copy link
Author

viviel commented Aug 11, 2020

Add test cases later

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 11, 2020
@viviel viviel force-pushed the #converter branch 2 times, most recently from 2e6af25 to 0aec98e Compare August 11, 2020 15:07
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Aug 11, 2020
@philwebb philwebb changed the title Converter supports lambda and method references Support lambda based converters by parsing bean method signature generics Aug 11, 2020
@philwebb philwebb added this to the 2.x milestone Aug 11, 2020
@philwebb philwebb self-assigned this Aug 11, 2020
@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Aug 11, 2020
@philwebb
Copy link
Member

philwebb commented Dec 9, 2020

We might want to look at the logic in ConfigurationPropertiesBean when we merge this.

@wilkinsona
Copy link
Member

@viviel We're still hoping to merge this when we have the time to do so. Would you be interested in re-opening it?

@wilkinsona wilkinsona removed this from the 2.x milestone Sep 10, 2021
@viviel
Copy link
Author

viviel commented Dec 1, 2021

@wilkinsona
I'm very sorry that I didn't pay attention to GitHub's email recently due to work reasons, so I didn't notice your message.
If necessary and the master branch does not solve this problem, I am happy to continue trying to solve it. Re sort out the cause of this problem and the code logic of PR. By the way, resolve the conflict.

@viviel viviel reopened this Dec 1, 2021
@wilkinsona
Copy link
Member

Thanks, @viviel. We'll see if we can merge this in 2.7.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Dec 2, 2021
@viviel
Copy link
Author

viviel commented Dec 4, 2021

I'd like to talk about the plan I expected. You can see if it can be implemented in this way. If so, I'll re implement it from the branch of 2.7 and submit it.

I've combed the code these two days and found that there are similar problems with four interfaces: Converter, Formatter, Printer and Parser. My idea is to uniformly modify the four interfaces. The specific scheme is to make an adaptation layer for each interface, put the generic information in it, and then uniformly use the void addConverter(GenericConverter converter) method to register.

I'll implement it according to this scheme, and then you can see the specific code logic. If not, adapt the current changes to the code of the 2.7 branch

@viviel viviel changed the base branch from main to 2.7.x December 4, 2021 09:42
@viviel viviel force-pushed the #converter branch 4 times, most recently from 00dde49 to 715167d Compare December 6, 2021 08:42
@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Dec 8, 2021
@philwebb philwebb added this to the 2.7.x milestone Dec 8, 2021
@philwebb philwebb modified the milestones: 2.7.x, 2.x Apr 11, 2022
@philwebb philwebb modified the milestones: 2.x, 3.x Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants