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

Geospatial commands #115

Open
kamyar1979 opened this issue Nov 8, 2018 · 12 comments
Open

Geospatial commands #115

kamyar1979 opened this issue Nov 8, 2018 · 12 comments

Comments

@kamyar1979
Copy link

Would you add geospatial commands? Can I help?

@k-bx
Copy link
Collaborator

k-bx commented Nov 8, 2018

Happy to review and merge a PR implementing them. Would you be willing to do one?

@kamyar1979
Copy link
Author

Sure! I would be happy to contribute!

@kamyar1979
Copy link
Author

PLease help me create a good PR! Should I fork the project first?

@k-bx
Copy link
Collaborator

k-bx commented Nov 8, 2018

@kamyar1979 yeah, I see the PR is a bit messed up because you had older fork probably. If your Git skills aren't very high -- I suggest completely deleting your fork first (go to kamyar1979/hedis repo page, and find "delete this repository" in settings), then fork this hedis repo, do your changes, commit and make a PR. This way it will be done on top of the latest commits.

@kamyar1979
Copy link
Author

I almost did the job. Still there is some remaining parts in georadius for implementing options, and also tests are not done. Today I will push a primary release.

@kamyar1979
Copy link
Author

kamyar1979 commented Nov 11, 2018

Pushed first release. Please help mw for adding options, I am confused. I guess I need a instance implementation of encode for GeoRadiusOpts but I am not sure.
Please check my fork and help me make it better! Also I have not yet added tests.

@kamyar1979
Copy link
Author

Sorry! I also did some minor fix and housekeeping in your code!

@k-bx
Copy link
Collaborator

k-bx commented Nov 12, 2018

@kamyar1979 where did you push it? You need to close this PR #116 and open a new one which will only have your new commits present

@kamyar1979
Copy link
Author

kamyar1979 commented Nov 12, 2018

I did it in my fork. I need you to check the code and then let me go on work before asking to merge.

@kamyar1979
Copy link
Author

I create a PR from my master to work-in-progress. I cant create a branch. Should I?

@k-bx
Copy link
Collaborator

k-bx commented Nov 12, 2018

@kamyar1979 please make a new branch that starts from the fresh master, once you will do everything correctly you should only see your commits in PR's commit list, not these >100 items seen in #117

Probably need to do something like

git remote add upstream [email protected]:informatikr/hedis.git
git fetch remote
git rebase remote/master
git push -f origin

if you don't feel comfortable doing this – please see my earlier suggestion on completely deleting the repo and re-forking (but be sure to backup the changes to reapply them)

@kamyar1979
Copy link
Author

My fork is new! I forked the repo just after your first comment!

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