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

fix JSONPath inconsistencies #1266

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix JSONPath inconsistencies #1266

wants to merge 3 commits into from

Conversation

N3XT14
Copy link

@N3XT14 N3XT14 commented Nov 11, 2024

@JyotinderSingh.

Sorry for the delay. I was busy with something urgent at work and couldn't give enough time.

Have a look at this PR. It's not completed yet but good portion of it is completed. Unfortunately, I am unable to find how to enable the resp3 protocol.

Also, in the Data Link shared the multi path examples seems incorrect in the array example.

Currently, I am testing it on my end but since certain parts such as RESP3 and custom formats are pending I am not asking for a merge yet.

I wanted to also open up a discussion on certain outputs that Redis returns such as for example "accessing negative index on out of bounds for an arrray example", etc.

Should we be following that as I believe we should instead return error for such cases instead of nil or the way redis handles.

Fixes #1002

@JyotinderSingh JyotinderSingh changed the title Issue 1002 fix JSONPath inconsistencies Nov 11, 2024
@JyotinderSingh
Copy link
Collaborator

We're not looking to support RESP3 at this stage. Let's stick with RESP2.

Is RESP3 support a blocker in any way?

@N3XT14
Copy link
Author

N3XT14 commented Nov 11, 2024

Nope, definitely not a blocker.

@JyotinderSingh
Copy link
Collaborator

Nope, definitely not a blocker.

In that case we can remove the related code and limit this PR to just the jsonpath changes.

@JyotinderSingh
Copy link
Collaborator

Looks like the tests and linter are failing, could you please take a look? Also, is this PR ready for review?

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

Successfully merging this pull request may close these issues.

Fix JSONPath inconsistencies in DiceDB
3 participants