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

GEOSEARCH & GEOSEARCHSTORE #2089

Merged
merged 10 commits into from
Apr 16, 2022
Merged

GEOSEARCH & GEOSEARCHSTORE #2089

merged 10 commits into from
Apr 16, 2022

Conversation

slorello89
Copy link
Collaborator

@NickCraver

This PR implements GEOSEARCH and GEOSEARCHSTORE for #2055

To abstract the box/circle sub-options from GEOSEARCH I added a new abstract class GeoSearchShape who's children are responsible for maintaining the bounding shape, its unit of measurement, the number of arguments required for the sub-option, and of course the sub-option name. Rather than casting/extracting the arguments, I have it use an IEnumerable state-machine with yield/return. Wasn't sure which was the better option, the IEnumerable seemed cleaner, open to whichever you want.

I changed the GEORADIUS pattern of having a GeoSearch(key, member, args. . .) and a GeoSearch(key, lon, lat, args. . .), and instead have GeoSearchByMember and GeoSearchByCoordinates. If I'm honest, it was because my IDE was complaining about breaking compatibility rules by having more than 1 override with optional parameters, not sure if you have a strong feeling on this.

Tried to maintain the single array-allocation pattern you added to #2075

slorello89 and others added 7 commits April 14, 2022 09:25
This matches GeoRadius today and while not as efficient, is much simpler to maintain in this high-args count case. We can always revisit, but relative to the cost of a search I'd opt for simpler code in this case.
@NickCraver
Copy link
Collaborator

@slorello89 Alrighty I hacked away at this a bit today - awesome work coming in. A few overalls:

  1. I appreciate the efficiency angle with array here, but when it's that complex and a relatively expensive op anyway - let's go for code simplicity (already changed)
  2. Method names mirroring GeoRadius is preferred, I'm guessing you had some intermediate problems along the way but current overloads shouldn't be an issue
  3. I didn't get a chance to review tests and check for null cases, etc. yet - will try to the next few days just slammed before vacation (I'm out next week btw - but will probably try to keep PRs going as able)

@slorello89
Copy link
Collaborator Author

slorello89 commented Apr 14, 2022

@NickCraver - regarding the overload names, I'm getting an RS0026 whenever I try to add anything lol, running into the same problem with ZMPOP

  IDatabase.cs(1874, 24): [RS0026] Symbol 'SortedSetPop' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details.

I think it's a either stylistic type warning that was somehow promoted to an error(maybe with all the nullable stuff?), or a bug in roslyn. I can suppress them and get it to build, but don't think that's particularly wise.

thoughts?

@NickCraver
Copy link
Collaborator

They'll go away once the API is added to PublicAPI.Shipped.txt, but they're sometimes legit...and sometimes bogus, I've hit a lot of cases with that analyzer. I'll be sure to poke ZMPOP more, but you shouldn't be seeing it in this PR!

@slorello89
Copy link
Collaborator Author

@NickCraver ahh jeeze - yep that was it, as soon as I added them to the shipped API vs the unshipped they were happy, odd but at least workable lol.

Copy link
Collaborator

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current is looking good - thanks!

@NickCraver NickCraver merged commit d81ec2c into main Apr 16, 2022
@NickCraver NickCraver deleted the feature/GeoSearch branch April 16, 2022 00:17
@NickCraver
Copy link
Collaborator

Dammit, forgot to check the Release notes - will add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants