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

Add value attribute on provider annotations #1522

Merged

Conversation

@kazuki43zoo
Copy link
Member

commented Apr 15, 2019

I propose to support the value attributes on provider annotations(such as @InsertProvider). In this change, developer can omit the type attribute as follow:

// @InsertProvider(type = InsertSqlProvider.class)  // until 3.5.1
@InsertProvider(InsertSqlProvider.class)
void insert(Name name);

Thetype attribute can use the alias for value attribute. This change related with gh-1279. The gh-1279 allow to omit method attribute, therefore this change is effective.

WDYT?

@kazuki43zoo kazuki43zoo self-assigned this Apr 15, 2019

@kazuki43zoo kazuki43zoo requested a review from harawata Jul 1, 2019

@harawata

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Thank you for your work, @kazuki43zoo !

I can see the benefit, so adding value attribute looks OK.
But I'm not sure about supporting two attributes for the same purpose.
I would rather deprecate the type attribute and remove it in a future release.

@jeffgbutler @h3adache Any opinion?

@kazuki43zoo

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Hi @harawata ,

Thanks for your quick reaction!!

But I'm not sure about supporting two attributes for the same purpose.
I would rather deprecate the type attribute and remove it in a future release.

I have other opinion for this topic.
I don't think that it is necessary to make deprecation on type attribute(alias attribute) because I know that support alias for value attribute on some famous framework(Spring Framework, Micronaut Framework).

WDYT?

@harawata
Copy link
Member

left a comment

Spring seems to have an auto-processing mechanism based on @AliasFor.
So no extra validation like this is necessary in Spring.

And I honestly don't see the benefit of providing an alias.
It makes users think 'which to choose?' or 'what is the difference?' even for a brief moment.

Anyway, this might be a matter of preference, so I'll approve.
We can add @deprecated later if we want. :)

@kazuki43zoo

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@harawata Thanks for approval.

We can add @deprecated later if we want. :)

OK. Let's think about good solution to be continued!!

@kazuki43zoo kazuki43zoo added this to the 3.5.2 milestone Jul 3, 2019

@kazuki43zoo kazuki43zoo merged commit 3b816f8 into mybatis:master Jul 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kazuki43zoo kazuki43zoo deleted the kazuki43zoo:support-alias-on-providerannotation branch Jul 3, 2019

kazuki43zoo added a commit that referenced this pull request Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.