-
Notifications
You must be signed in to change notification settings - Fork 122
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
Geohash Integer methods #56
Comments
@toin0u Any opinion on this? |
Hi @mablae! Sorry to reply so late. I think I'll go to refactor the current Geohash class :) Are you willing to help with this? |
Sure, I am. I already looked at the current implementation and noticed that Geohash maintains its state internally as geohash string. How would you refactor the class in a sane way? EDIT: Another thing is the internal handling of long integer values in PHP when printing them out. When putting geohashes into Redis, I had to work around this issue by using number format. Should it be handled by the GeoInteger methods, meaning it would return an string? Or is returning real integer better and let the users decide, depending their usecase? http://stackoverflow.com/questions/9630793/how-to-display-long-integers-on-browser-in-php |
For a personal project I converted the algorithm used by https://github.com/sunng87/node-geohash Since there is also #3 (Geohash36) maybe it would be the better way to have a GeoHashInterface and then multiple self containing implementations on that? This would be easier to extend and no BC breaks at all.. Converting between different geohashes would also be easily possible then... What do you think? |
@mablae I agree with you :) The interface is a good idea. I'm 👍 for that. Thanks for pointing out the issue with the long integer.. I think you're right to return the string make sense in this case. Thank you very much for commenting and come with inputs, I appreciate it :) |
Okay, fine. 👍 I will prepare a PR on the weekend to be discussed further then. |
That's sound nice! I'm looking forward to it :) |
@mablae any news about this? :) |
Any news about integer version of Geohash? |
It would be quite nice to allow encoding into geohash integers.
For example https://github.com/sunng87/node-geohash offers the
encode_int = function (latitude, longitude, bitDepth)
function.Since the current
Geohash
implemenation manages state inside the class and is not like a static converter class, the way to go would be a newGehashInteger
class, right?Or does it make sense to refactor the the current
Geohash
class to support additional converting methods for geohash integers?EDIT: Better description
cheers mablae
The text was updated successfully, but these errors were encountered: