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

HSEARCH-2188 Fixed type checking in NumericFieldUtilsTest #1046

Closed
wants to merge 1 commit into from

Conversation

gfouquet
Copy link
Contributor

isAssignableFrom in NumericFieldUtils was used the wrong way. I turned it around and added a unit test, which did not pass with the previous implementation.

@Hibernate-CI
Copy link
Contributor

Can one of the admins add this person to the trusted builders? (reply with: "add to whitelist" or "ok to test")

@Sanne
Copy link
Member

Sanne commented Mar 16, 2016

ok to test

@Sanne
Copy link
Member

Sanne commented Mar 16, 2016

thanks @gfouquet ! I'll gladly apply your patches, but could you please sign the Hibernate contribution agreement?
It's as simple as creating an account, reading the terms and clicking on a checkbox: https://cla.jboss.org

@gfouquet
Copy link
Contributor Author

Hi,
I just signed the CLA

value instanceof Long ||
value instanceof Integer ||
value instanceof Float ||
value instanceof Calendar;
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I'm surprised this is not consistent with the code above. Shouldn't we also have Byte, Date and Short here?

Copy link
Member

Choose a reason for hiding this comment

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

+1. Sounds reasonable to me to have Byte, Date and Short here as well.

@gunnarmorling
Copy link
Member

Where are we with this one?

@gfouquet
Copy link
Contributor Author

I'm willing to make any change the Hibernate team see fit. On the other hand I'm mostly clueless about the side effects of any change.

@gsmet
Copy link
Member

gsmet commented Apr 13, 2016

@gunnarmorling it's already fixed in master for a while and I extracted the test case and the additional cleanup from Gregory's PR and pushed it to master: a5ad4f5

I asked @Sanne if we still maintain 5.3 and 5.4 but I haven't had a reply so far.

If we do, I'll prefer to only cherrypick Davide's commit fixing this: 4c0415c . No need for the further cleanup in the old branches.

@gsmet
Copy link
Member

gsmet commented Apr 27, 2016

@gfouquet I will backport Davide's fix in 5.3 and 5.4. That being said, we haven't planned to release new versions of these 2 branches.

@gsmet
Copy link
Member

gsmet commented Apr 27, 2016

OK, so I pushed the test and the cleanup to master a few weeks ago and backported the original Davide's fix to 5.3 and 5.4 just now.

Closing. Thanks!

@gsmet gsmet closed this Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants