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.

Sign up for GitHub

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

Jump to bottom

Polish #24862

Closed
wants to merge 1 commit into from
Closed

Polish #24862

wants to merge 1 commit into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Jan 18, 2021

This PR changes to configure jOOQ ConnectionProvider via customizer as it seems to be the original intention based on #24732.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 18, 2021
@snicoll
Copy link
Member

snicoll commented Jan 18, 2021

Ah ha. Good catch there @izeye, I forgot that one.

@snicoll snicoll added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 18, 2021
@snicoll snicoll added this to the 2.5.0-M1 milestone Jan 18, 2021
@snicoll snicoll self-assigned this Jan 18, 2021
@snicoll
Copy link
Member

snicoll commented Jan 18, 2021

Actually, let me take that back. I did that on purpose, but didn't do it properly anyway.

The plan with the customizer was to only customise things that have been defined externally. I didn't configure ConnectionProvider this way as the plan is to deprecate the customizer eventually and that would stop configuring the ConnectionProvider that we auto-configure ourselves.

Re-reading the code, I noticed we also auto-configure a DefaultExecuteListenerProvider, so the processing of ExecuteListenerProvider should rather move to the main auto-configuration for the same reason.

Would you be willing to review your PR in that direction?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 18, 2021
@izeye izeye changed the title Configure jOOQ ConnectionProvider via customizer Polish Jan 18, 2021
@izeye
Copy link
Contributor Author

izeye commented Jan 18, 2021

@snicoll Thanks for the feedback! I missed it. I updated as you suggested.

@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 Jan 18, 2021
snicoll pushed a commit that referenced this pull request Jan 18, 2021
@izeye @snicoll
Polish
3aea8b3 
See gh-24862
snicoll added a commit that referenced this pull request Jan 18, 2021
@snicoll
Polish contribution
dcc0ca0 
See gh-24862
@snicoll snicoll closed this in 38cd1b4 Jan 18, 2021
@snicoll
Copy link
Member

snicoll commented Jan 18, 2021

Thanks for the quick feedback Johnny! I've polished by adding assertions outside of that deprecated test to make it more obvious those two providers will be honoured once we remove the customizer (see dcc0ca0).

@izeye izeye deleted the jooq branch January 18, 2021 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@snicoll snicoll

Labels
status: feedback-provided Feedback has been provided type: task A general task
Projects
None yet
Milestone
2.5.0-M1
Development

Successfully merging this pull request may close these issues.
3 participants
@izeye @snicoll @spring-projects-issues