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

Extend Geospatial support by implementing GEOSEARCH command #867

Merged
merged 22 commits into from
Dec 28, 2022

Conversation

vladvildanov
Copy link
Contributor

@vladvildanov vladvildanov commented Dec 15, 2022

@chayim chayim marked this pull request as draft December 15, 2022 09:44
@vladvildanov
Copy link
Contributor Author

@tillkruss Hey. Till! I want you to check this PR and let me know what you're thinking about it

In this PR I introduce an idea to represent complex command arguments as objects to make it easy handle them and provide more useful interface for developers to use such commands.

By "complex command arguments" I mean a conditional arguments that can accept different values (For example FROM and BY arguments in GEOSEARCH command). To avoid adding all of them as optional arguments in PHP and avoid problems with processing them, I introduce an interfaces and objects that allows to simplify usage for clients and simplify argument processing for developers as well (In my opinion).

Let me know what you think and is it a good idea to use objects to represent command arguments

@vladvildanov vladvildanov changed the title [WIP] Extend Geospatial support by implementing GEOSEARCH command Extend Geospatial support by implementing GEOSEARCH command Dec 21, 2022
@vladvildanov vladvildanov marked this pull request as ready for review December 21, 2022 15:15
@vladvildanov vladvildanov linked an issue Dec 22, 2022 that may be closed by this pull request
@vladvildanov vladvildanov marked this pull request as draft December 22, 2022 09:59
@tillkruss
Copy link
Member

@vladvildanov I'll take look in the new year. It's a rather big change for a patch release, this should probably be a minor release at least.

@chayim
Copy link
Contributor

chayim commented Dec 26, 2022

@vladvildanov Why'd you move this back to draft? Command-wise and implementation wise, this looks to properly cover the GEO* options and command set, thank you. Like @tillkruss said.. looks like we're getting closer to releasing with a bump.

@vladvildanov
Copy link
Contributor Author

@chayim Just because it's not fully covered with tests (f.e Argument objects). According to functionality itself, it's ready

@vladvildanov vladvildanov linked an issue Dec 27, 2022 that may be closed by this pull request
@tillkruss tillkruss marked this pull request as ready for review December 28, 2022 17:09
@tillkruss tillkruss merged commit bb65b31 into predis:main Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Add support for missing redis command GEOSEARCH GEOSEARCH / GEOSEARCHSTORE not in predis
3 participants