Skip to content

Throw Exception when adding @AutomapConstructor to multiple constructors #2627

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

Conversation

sogoagain
Copy link
Contributor

@sogoagain sogoagain commented Jul 26, 2022

Fixes #2626

Provides helpful error messages when adding @AutomapConstructor to multiple constructors.

thank you.

…nstructor can only be used in one constructor.
Copy link
Member

@harawata harawata left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, @sogoagain !

Actually, this problem is not affected by the argNameBasedConstructorAutoMapping setting, so could you please move the test case to org.apache.ibatis.autoconstructor ?

If you are busy, please just let me know and I'll take care of it when I have time.

throw new ExecutorException("@AutomapConstructor should be used in only one constructor.");
}
if (automapConstructors.size() == 1) {
return Optional.of(automapConstructors.get(0));
}
if (configuration.isArgNameBasedConstructorAutoMapping()) {
Copy link
Member

Choose a reason for hiding this comment

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

How about using reduce?

return Arrays.stream(constructors)
    .filter(x -> x.isAnnotationPresent(AutomapConstructor.class))
    .reduce((x, y) -> {
  throw new ExecutorException("@AutomapConstructor should be used in only one constructor.");
}).or(() -> {
  if (configuration.isArgNameBasedConstructorAutoMapping()) {
     ...

It probably is a matter of preference, so if you don't like it, the current version is fine. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks very good! thank you.

@sogoagain sogoagain changed the title Throw Exception when using @AutomapConstructor multiple times with argNameBasedConstructorAutoMapping setting Throw Exception when using @AutomapConstructor multiple times Jul 30, 2022
@sogoagain sogoagain changed the title Throw Exception when using @AutomapConstructor multiple times Throw Exception when adding @AutomapConstructor to multiple constructors Jul 30, 2022
@sogoagain
Copy link
Contributor Author

@harawata Thanks for the review!

I understood and moved the test code package. also edited the PR title and content.

@sogoagain sogoagain requested a review from harawata July 30, 2022 09:07
sogoagain and others added 2 commits July 31, 2022 12:05
@coveralls
Copy link

coveralls commented Jul 31, 2022

Coverage Status

Coverage decreased (-0.006%) to 87.316% when pulling 68447e2 on sogoagain:arg-name-based-constructor-automapping-only-one-automap-constructor into 034cacd on mybatis:master.

@harawata harawata merged commit e73e023 into mybatis:master Jul 31, 2022
@sogoagain sogoagain deleted the arg-name-based-constructor-automapping-only-one-automap-constructor branch July 31, 2022 07:33
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.

Helpful error message when using multiple @AutomapConstructor with argNameBasedConstructorAutoMapping setting
3 participants