Skip to content

Convert AggregateReference by converters instead of custom code paths. #993

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
Closed
wants to merge 3 commits into from
Closed

Convert AggregateReference by converters instead of custom code paths. #993

wants to merge 3 commits into from

Conversation

@schauder
Copy link
Contributor

@schauder schauder commented Jun 23, 2021

Closes #992

@schauder schauder requested a review from mp911de June 23, 2021 14:07
@schauder schauder force-pushed the issue/992-agg-ref-converters branch from 887f17b to c55690f Compare July 1, 2021 11:51
mp911de
mp911de requested changes Jul 1, 2021
View reviewed changes
spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/QueryMapper.java Outdated
* @param value the value to be converted.
* @return a non null {@link JdbcValue} holding the converted value and the appropriate JDBC type information.
*/
private JdbcValue convertSpecial(RelationalPersistentProperty property, @Nullable Object value) {
Copy link
Member

@mp911de mp911de Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having special in a method name is typically a red flag. How about something like getWriteValue?

Copy link
Contributor Author

@schauder schauder Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think getWriteValue really conveys what the method is doing. And it is handling the special values.

I agree that the special is suspicious and there might be a bette abstraction hiding or at least a better name, I don't think getWriteValue is it.

@schauder schauder force-pushed the issue/992-agg-ref-converters branch from c55690f to 887f17b Compare July 1, 2021 12:15
@schauder schauder mentioned this pull request Jul 1, 2021
Closed
@mp911de mp911de self-assigned this Jul 14, 2021
mp911de pushed a commit that referenced this pull request Jul 15, 2021
@schauder @mp911de
Convert AggregateReference by converters instead of custom code paths.
221d113 
Closes #992
Original pull request #993
mp911de added a commit that referenced this pull request Jul 15, 2021
@mp911de
Polishing.
4f4014d 
Hide actual converters by making these private classes, expose factory method to construct converters. Avoid stream API usage on converter code paths. Remove unused imports. Clean up tests.

Original pull request: #993.
See #992
@mp911de mp911de added this to the 2.3 M1 (2021.1.0) milestone Jul 15, 2021
@mp911de mp911de added the type: enhancement A general enhancement label Jul 15, 2021
@mp911de
Copy link
Member

mp911de commented Jul 15, 2021

That's merged and polished now.

@mp911de mp911de closed this Jul 15, 2021
@mp911de mp911de deleted the issue/992-agg-ref-converters branch July 15, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mp911de mp911de mp911de requested changes

Assignees

@mp911de mp911de

Labels

type: enhancement A general enhancement

Projects

None yet

Milestone

2.3 M1 (2021.1.0)

Development

Successfully merging this pull request may close these issues.

Convert AggregateReference using Converters instead of custom code paths

3 participants

@schauder @mp911de