Skip to content

Conversation

@xiaoma20082008
Copy link

add new configuration to support the java's method overload

@harawata
Copy link
Member

harawata commented Oct 1, 2018

Thank you for the PR, @xiaoma20082008 !

Most users who requested overloaded methods seemed to want to map multiple overloaded methods to a single XML statement (e.g. #705 ).
For example, mapping the following two methods...

User findUser(Integer id);
User findUser(Integer id, String name);

...to this XML statement.

<select id="findUser" resultType="User">
  select from users where id = #{id}
  <if test="name != null">
    and name = #{name}
  </if>
</select>

It is already possible with Java 8's default methods.

default User findUser(Integer id) {
  return findUser(id, null);
}
User findUser(Integer id, String name);

This PR, however, maps a different XML statement for each overloaded method.
Is this really what you want?
Isn't it better to name these methods differently in such case?

@xiaoma20082008
Copy link
Author

Thanks for your replying @harawata .
Because of some developers in my company use this future under jdk7, so I developed this patch also I'm pushing them to use a different method name but it's slowly.
If most users don't need this patch, Please close this pr ,Thank you!

@harawata
Copy link
Member

harawata commented Oct 9, 2018

Thanks for the explanation, @xiaoma20082008 !

So, it is a kind of special case and not many users are in the same situation, I guess.
I'll close this PR for now, but will keep watching requests.

Thanks again for taking the time to submit the PR!

Note to self:
In case we support overloaded methods in the future, this approach (auto-generating statement ID from method name and parameter types) makes IDs too verbose, in my opinion.
It might be better to let users assign a new statement ID explicitly via some annotation (e.g. @StatementId("newName")).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants