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

Scanner for name repository is problematic when constants values are not unique #31

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

Conversation

andreacalia
Copy link
Member

Found out when building on Travis with jdk9. If the package scan finds, e.g., 2 constants with the same value, the key of the internal map will be the same overriding one of them..

How could we improve this? This pull request is just for not forgetting about this :)

}

@Test
public void testDuplicateConstantEntry() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the test that should work after the improvements

@michi42
Copy link
Member

michi42 commented Jul 12, 2018

I don't think there is a general solution for this problem - how shall we know which constant is the "right" one? Unless we want to work somehow with annotations (e.g. qualifiers) I think the best would be to force exactly one match or else throw. There could be another method which never throws but returns a Set of names.

@andreacalia
Copy link
Member Author

Yes, there isn't a clear option in my opinion. We anyway have to choose one, I think throwing is a good option since at least you get a nice error message, but then we have to restrict the packages to scan, because it will throw while scanning Java packages out of our control (this it exactly what was happening).

@kaifox
Copy link
Member

kaifox commented Jun 19, 2019

Probably a solution could be to not throw when scanning, but keep the set in the repo. However, when querying we could have several methods (There one has to know what to expect and it is potentially a better moment to throw...). E.g. :

repo.names(value); // would return a set
repo.name(value); // would return the name or throw if none or more than one.
repo.optionalName(value); // would return an empty optional if no name available, a filled optional if the name is present or throw if more names are available.

What would you think?

@andreacalia
Copy link
Member Author

Probably a solution could be to not throw when scanning, but keep the set in the repo. However, when querying we could have several methods (There one has to know what to expect and it is potentially a better moment to throw...). E.g. :

repo.names(value); // would return a set
repo.name(value); // would return the name or throw if none or more than one.
repo.optionalName(value); // would return an empty optional if no name available, a filled optional if the name is present or throw if more names are available.

What would you think?

Agree :)

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