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

Update to CityHash v1.1 #7

Open
rmg opened this issue Apr 17, 2013 · 6 comments
Open

Update to CityHash v1.1 #7

rmg opened this issue Apr 17, 2013 · 6 comments

Comments

@rmg
Copy link
Contributor

rmg commented Apr 17, 2013

I've got a branch that updates CityHash to v1.1, but all of the tests fail. I think it is because the hash functions have actually changed and the tests are still valid, but I'm not 100% certain if that's expected or not.

If it is, I'll just update the tests and submit a pull request.

@yyfrankyy
Copy link
Contributor

Yes, there is some compatibility issue, but I am sure that old version is still needed.

If you can send a PR with compatibility in both version, I think @fbzhong will happy to merge.

@rmg
Copy link
Contributor Author

rmg commented Apr 19, 2013

@yyfrankyy do you mean include both versions of the CityHash C++ library in the module?

@yyfrankyy
Copy link
Contributor

Yes.

@rmg
Copy link
Contributor Author

rmg commented Apr 19, 2013

Hmm.. interesting idea.

Here's a few options that come to mind

var cityhash = require('cityhash')
  , hash64 = cityhash.hash64           // default to 1.0.3
  , cityhash11x = cityhash.v11x
  , new_hash64 = cityhash.v11x.hash64   // allow access to 1.1.0

Or maybe something like:

var cityhash10x = require('cityhash')         // default to 1.0.3
  , cityhash11x = require('cityhash/1.1.0')   // allow access to 1.1.0

I'm not a fan of using config strings, but it's also an option:

var cityhash = require('cityhash')
cityhash === cityhash.engine('1.0.3')      // top level == existing API
cityhash.engine('1.1.0').hash64("something to hash with 1.1.0")
cityhash.engine('1.0.3').hash64("something to hash with 1.0.3")
cityhash.hash64("uses 1.0.3 at top level to maintain compatibility")

Any preferences for the API?

@yyfrankyy
Copy link
Contributor

Option 2 and 3 looks good to me. or we can just release a new version of citihash (1.x) to npm, and stop adding new feature to 0.x version (just bugfix).

@rmg
Copy link
Contributor Author

rmg commented Apr 20, 2013

Oh, right.. I always forget about maintaining multiple versions! I'll keep an eye out fro @fbzhong to make a branch and submit a pull request when I see it.

@rmg rmg mentioned this issue Apr 27, 2013
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

No branches or pull requests

2 participants