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

Optimize the code in TypeAliasRegistry #2842

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kang-hl
Copy link
Contributor

@kang-hl kang-hl commented Mar 8, 2023

Optimize the code in TypeAliasRegistry

@hazendaz hazendaz self-assigned this Mar 11, 2023
@hazendaz
Copy link
Member

@kang-hl There is a test shouldBeAbleToRegisterAliasWithNullType that fails with this. I haven't looked deeper but that would indicate null was allowed and thus the changes would break that logic. Please have a look at that test specifically to see if it indicates why usage is that way. For now cannot merge this change. The change itself seems fine but I don't know the deeper details as to why null allowed.

@kang-hl
Copy link
Contributor Author

kang-hl commented Mar 13, 2023

@kang-hl There is a test shouldBeAbleToRegisterAliasWithNullType that fails with this. I haven't looked deeper but that would indicate null was allowed and thus the changes would break that logic. Please have a look at that test specifically to see if it indicates why usage is that way. For now cannot merge this change. The change itself seems fine but I don't know the deeper details as to why null allowed.

fixed

@hazendaz
Copy link
Member

I seemed to recall there was another section with updates...At any rate I don't think the else is necessary and think the first two if statements can be combined so its basically like the original just optionized to not call the get so many times as the intent here.

@coveralls
Copy link

Coverage Status

Coverage: 87.547% (+0.001%) from 87.545% when pulling fb2a6e9 on kang-hl:Optimize-TypeAliasRegistry into e68216b on mybatis:master.

@kang-hl
Copy link
Contributor Author

kang-hl commented Mar 14, 2023

I seemed to recall there was another section with updates...At any rate I don't think the else is necessary and think the first two if statements can be combined so its basically like the original just optionized to not call the get so many times as the intent here.

Better performance in test shouldBeAbleToRegisterSameAliasWithSameTypeAgain.

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.

3 participants