Add geo add #489
Add geo add #489
Conversation
…ed to fix GeoAdd [] command. Added in GeoDist command and tested using the example online, also needs unit tests. Added in geohash command - needs unit tests. Added geo pos - needs tests. started work on tests - moved new types to new files. Spelling fix. Replaced 'paramter' with 'parameter' Fixed build error caused by TryGetAzureRoleInstanceIdNoThrow test Refactoring. Implemented GeoPosition type and methods. Updated naming of geodistance created types needed for geo radius command. working on GeoRadius method. - needs some more testing.
|
Also, I'm not real sure why Appveyor is having a hard time with the build, as I'm not able to reproduce this behavior locally... |
|
Hi, I'd like to participate to this request. First of all we've installed Redis 3.2 to get the new GEO features, and we're using your great library. Therefore we'd like to get them in the master as soon as possible. What is your road map to merge this request into the master ? In the meanwhile we made some tests, and we found an issue with one parameter (COUNT) in the GEORADIUS command. Another question raise here : till the branch is not merged to the master, we'd like to produce our own local nugget package, so can you tell me some clue to create that package ? Do you have any script or do you that manually ? Regards, Eric |
|
Not sure where the pain lies but I'm could lend a hand if it would be useful. |
|
Hi guys, Looking into this compilation issue, I've found what's wrong. There are missing files. Look at this screen shot, the file is missing in commit bc2ec48 The file was removed on commit : bc2ec48 Hope that helps, Eric |
|
@Wallonman Indeed - unfortunately for some reason this PR makes massive changes to the I expect we should probably upgrade to the VS 2017 format in the next RC. It's coming along just not quite there for stable projects yet. The issue is that it excludes contributors without VS 2017 from working...it's a sharp cutover that's not at all open source friendly. However, it avoids most to all of the project file conflicts like this. |
|
If I understand well, it's a matter of VS version ? Or maybe simpler : fixing the csproj ! |
|
I've pushed the fix on my branch : https://github.com/Wallonman/StackExchange.Redis Someone can try to build on the CI ? |
|
@NickCraver - I cant really speak as to why the file refs changed. It isnt something I specifically recall doing, nor do I know why I would have had a reason to. I do recall it giving me headaches when I added in new files though. Perhaps something I did when adding new files caused a problem. |
|
When saving the project with VS 2013 then the csproj is modified and the
wildcard is replaced by the list of files included in the projet.
This should not be an issue, you just must take care to - when you add or
remove files from the projet - commit the added/removed file to git in
order to be synced
…____________________
Eric Piraux
+32 476 20 29 60
2016-12-29 20:18 GMT+01:00 William Davis <[email protected]>:
@NickCraver <https://github.com/NickCraver> - I cant really speak as to
why the file refs changed. It isnt something I specifically recall doing,
nor do I know why I would have had a reason to. I do recall it giving me
headaches when I added in new files though. Perhaps something I did when
adding new files caused a problem.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#489 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AOUSb6QE3D0hTPjPj9efdkfXoEXl7Na3ks5rNAeAgaJpZM4J_sr6>
.
|
|
Having a look through now; note - unless I hit a brick wall, I'll try to make any fixes myself locally - any comments will be purely informational unless indicated otherwise; will add them here to track (because the review is already hella confusing) So far:
|
….Redis into wjdavis5-AddGeoAdd Apply feedback from #489 (comment) - note still untested; that's next # Conflicts: # StackExchange.Redis.Tests/ConnectionFailedErrors.cs # StackExchange.Redis/StackExchange/Redis/IDatabase.cs # StackExchange.Redis_Net40/StackExchange.Redis_Net40.StrongName.csproj # StackExchange.Redis_Net40/StackExchange.Redis_Net40.csproj # StackExchange.Redis_Net45/StackExchange.Redis_Net45.StrongName.csproj # StackExchange.Redis_Net45/StackExchange.Redis_Net45.csproj # StackExchange.Redis_Net46/StackExchange.Redis_Net46.StrongName.csproj # StackExchange.Redis_Net46/StackExchange.Redis_Net46.csproj
|
my changes are visible in this branch (until I delete it) for your reference; https://github.com/StackExchange/StackExchange.Redis/commits/wjdavis5-AddGeoAdd so far I've only looked at the code; putting it through its paces shortly. Thanks for the PR - much appreciated. Sorry it has taken some time and some iteration, but I'm ... fussy. More specifically, it is virtually impossible to fix any API problems once we've released them, so it needs to be "right enough" first time out of the gate. |
|
woohoo; tests added for all methods, and merged; thanks for the contribution |
4 participants
I've went through and made most of the suggested changes, I didnt remove the geo radius member yet. I'm trying to remember what I was thinking at the time I put it in there. Nonetheless comments are welcome as always.
I'm hoping to get this finished up sometime in the near future.