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

Hash code builder #10

Closed
wants to merge 2 commits into from
Closed

Hash code builder #10

wants to merge 2 commits into from

Conversation

mureinik
Copy link
Contributor

@mureinik mureinik commented Sep 3, 2013

Solution for LANG-804: Redundant check for zero in HashCodeBuilder ctor

@kelvinst
Copy link

Nice PR, I'd like the test added and the rename of them to clarify what they really tests...
Maybe add some tests with odd values tests, positive and negative, will be great 😃

@britter
Copy link
Member

britter commented Sep 20, 2013

@mureinik I'd like to apply your changes to my svn working copy for review but I'm having troubles to generate an appropriate diff file. Do you know how to do the trick?

@mureinik
Copy link
Contributor Author

@kelvinst I'd be happy to add more tests, but I'm not quite clear what you're suggesting. Just adding one-liners that make sure that new HashCodeBuilder(-17, -37) does not throw an IllegalArgumentException?
Something more?

Note that HashCodeBuilderTest is full of calls to new HashCodeBuilder(17, 37), so we pretty much have that covered :-)

@mureinik
Copy link
Contributor Author

@britter it's been a while since I worked with SVN, but this should do the trick:
https://github.com/apache/commons-lang/pull/10.diff (note that this includes both patches - the test enhancement/cleanup and the LANG-804 solution).

Alternatively, if you pull to a local git you could just do something like this and create the patches locally:
git format-patch -n2 -o /tmp/HashCodeBuilder

Does this do the trick for you?

@britter
Copy link
Member

britter commented Sep 27, 2013

@mureinik I've applied your patch in r1526817. Thanks for contributing!

@mureinik
Copy link
Contributor Author

@britter thanks for applying it. Happy to help :-)

@kelvinst
Copy link

Sorry, I didn't saw that other tests ;D... This can be close now, right?

@mureinik
Copy link
Contributor Author

@kelvinst didn't notice the request stayed open, thanks.
Closing.

@mureinik mureinik closed this Sep 27, 2013
@mureinik mureinik deleted the HashCodeBuilder branch September 27, 2013 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants