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

Shared ambiguity instance #3146

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

Conversation

nieqiurong
Copy link
Contributor

When a conflicting key is encountered when generating a short key, we can share an instance to reduce memory usage.

@coveralls
Copy link

coveralls commented Apr 18, 2024

Coverage Status

coverage: 87.174% (-0.003%) from 87.177%
when pulling 4a9d42c on nieqiurong:shared-ambiguity
into b5236ec on mybatis:master.

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, @nieqiurong ,

I could be wrong, but this seems to make the error message less informative.
Please add a test case.

@harawata
Copy link
Member

Ah, I see, you are now using key instead of .getSubject().
In that case, we should remove subject from Ambiguity, or am I still missing something?

@epochcoder
Copy link
Contributor

imo, if we are going to throw an exception when an ambiguous key is detected, we might as well remove the inner class entirely and do that where it is detected, as essentially this just makes it a kind of a maker interface and getSubject() useless, but agree that there definitely needs to be a test case for this.

@nieqiurong
Copy link
Contributor Author

Yes, because the key is originally the value that was put in. When the key value is repeated, we can use a fixed value as the value, which can be a specific Object.

@nieqiurong
Copy link
Contributor Author

I'll modify it later.

@harawata
Copy link
Member

I think we can remove Ambiguity immediately.
It's clearly internal.

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.

4 participants