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

Node: add GEOSEARCH #2007

Merged

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented Jul 24, 2024

Refs: #1482 and #1685

And update docs for java and python commands.

Includes some changes from #2005

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
…-GEOSEARCH-valkey-90

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand added the node Node.js wrapper label Jul 24, 2024
@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review July 25, 2024 00:48
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner July 25, 2024 00:48
*
* @param key - The key of the sorted set.
* @param searchFrom - The query's center point options, could be one of:
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious...
Are we going to have extra line for each sub-item?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is needed for line breaks in the list. Withot that, all bullet points are rendered in one messy line without a list formatting.

node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Commands.ts Outdated Show resolved Hide resolved
Co-authored-by: Yi-Pin Chen <yi-pin.chen@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@GumpacG GumpacG self-requested a review July 25, 2024 17:37
CHANGELOG.md Show resolved Hide resolved
node/src/Commands.ts Outdated Show resolved Hide resolved
): command_request.Command {
let args: string[] = [key];

if ("position" in searchFrom) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a different check here? Checking for string seems incorrect here.

Copy link
Collaborator Author

@Yury-Fridlyand Yury-Fridlyand Jul 25, 2024

Choose a reason for hiding this comment

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

It checks for the named property/field in the variable. typeof/instanceof doesn't work there unfortunately. Do you have a better idea how to do a runtime type check?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the standard way to do it

node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Commands.ts Outdated Show resolved Hide resolved
): command_request.Command {
let args: string[] = [key];

if ("position" in searchFrom) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the standard way to do it

node/tests/SharedTests.ts Outdated Show resolved Hide resolved
@Yury-Fridlyand Yury-Fridlyand marked this pull request as draft July 26, 2024 17:36
@Yury-Fridlyand
Copy link
Collaborator Author

Yury-Fridlyand commented Jul 26, 2024

deleted

@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review July 26, 2024 23:26
@Yury-Fridlyand Yury-Fridlyand merged commit b753216 into valkey-io:main Jul 27, 2024
8 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the node/yuryf-GEOSEARCH-valkey-90 branch July 27, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node Node.js wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants