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 implementation of hashCode method for Dynamic Value along with the corresponding test #696

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

BijenderKumar1
Copy link

@BijenderKumar1 BijenderKumar1 commented Jun 25, 2024

/claim #655
In order to resolve the issue of incosistent hashCode values for DynamicValue objects, I have added an implementation of the hashCode method for the DynamicValue class. I have also added a test to ensure that the hashCode values are consistent. Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes This is ready for review. Thanks!

@jdegoes
Copy link
Member

jdegoes commented Jun 28, 2024

Unfortunately, the job is bigger than just hash code. You also need to fix equality. In addition, the test suite needs to be more comprehensive, verifying that different ways of creating equivalent dynamic value compare and hash equal.

I will increase the bounty on this one as there is more work than I thought.

@BijenderKumar1
Copy link
Author

@jdegoes I have fixed equality as well by overriding the equals method for DynamicValue. I have also expanded the text suite to incorporate checks for different ways of creating equivalent dynamic values. Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes Just a gentle reminder to please review this! Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes Really sorry to bother you! Just a gentle reminder. Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes Just a gentle reminder! Thanks!

@jdegoes
Copy link
Member

jdegoes commented Jul 15, 2024

@BijenderKumar1 The tests are not testing anything robust.

A robust test will be property-based, using dynamically generated DynamicValue, without involvement of Schema or specific classes, and will test for consistent hash and equality.

@BijenderKumar1
Copy link
Author

@jdegoes Thanks for the review. I have added a new function to dynamically generate DynamicValues and added tests for consistent hash and equality. Thanks!

@BijenderKumar1
Copy link
Author

BijenderKumar1 commented Aug 1, 2024

@jdegoes Just a gentle reminder to review this. Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes Just a gentle reminder to review this. Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes Just a gentle reminder to review this. Really sorry to bother you repeatedly! Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes Hope you are doing well! Just a gentle reminder for reviewing this. Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes @987Nabil Just a gentle reminder to please review this. I will be really thankful!

@987Nabil
Copy link
Contributor

987Nabil commented Sep 2, 2024

@BijenderKumar1 There are still issues. I'll write once I have the time for it

@BijenderKumar1
Copy link
Author

@987Nabil No worries! I will be super happy to incorporate your suggestions. Thanks!

@BijenderKumar1
Copy link
Author

@jdegoes Hope you are doing well! Just a gentle reminder to please review this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants