Command Migration: JSON. ARRAPPEND, ARRPOP, ARRLEN

[srivastava-yash]

This PR migrates JSON.ARRAPPEND, JSON.ARRPOP, JSON.ARRLEN to make it protocol agnostic. Closes #1026

Migration Checklist

• Migrated the evalXXX function with the latest definition
• Update or add unit tests for the new implementation.
• All unit tests pass successfully.
• Ensure all integration tests pass successfully.
• Ensure integration tests are added for the migrated command on multi-threaded resp server.
• Move Integration tests for the respective commands under the RESP integration tests directory from Async directory
• Please validate that the documentation for the respective commands is up to date. If not then consider adding them.

[AshwinKul28]

@srivastava-yash Thanks for the PR.
Can you please check the failed test cases?

[srivastava-yash]

@srivastava-yash Thanks for the PR. Can you please check the failed test cases?

yes @AshwinKul28, working on it.

[srivastava-yash]

The integration tests for the commands ARRAPPEND and ARRPOP seem to be failing because of the same reason, when ever the response is to be nil its getting a different string for nil. As seen in the attached screenshot the expected is (nil) but its getting nil with < > . Changing expected to < nil > does not fix the test case either. Still finding reason for this.

[AshwinKul28]

Thanks a lot @srivastava-yash for the changes. Left few comments.

[soumya-codes]

@srivastava-yash lets ensure that the corresponding integration tests are migrated as well. cc @AshwinKul28

[soumya-codes]

Just FYI, we need to add the tests for the migrated commands under integration_tests/commands/resp

I have added a new item to checklist in the PR description. Please tick the same once the integration tests are added.
Please let me or Ashwin know if you have any doubts/concern.

[lucifercr07]

@srivastava-yash please resolve the conflicts once.

[srivastava-yash]

@srivastava-yash please resolve the conflicts once.

working on it
Done @lucifercr07

[AshwinKul28]

@srivastava-yash, please consider working on other items on the checklist. I know it's a bit more effort, but since you are the best person to work on these complimentary tasks related to these commands. Thanks

[lucifercr07]

@srivastava-yash please resolve the conflicts.

[srivastava-yash]

@srivastava-yash please resolve the conflicts.

Done @lucifercr07

[srivastava-yash]

@lucifercr07 @AshwinKul28 any update on how we are going to move forward with this task?

[lucifercr07]

@srivastava-yash HTTP issue has been fixed as part of #1150, is there any other blocker for this?

[srivastava-yash]

@srivastava-yash HTTP issue has been fixed as part of #1150, is there any other blocker for this?

Thanks @lucifercr07, I missed the PR. I will resolve conflict and fix integration tests

[srivastava-yash]

@lucifercr07 @AshwinKul28 PR is up for review again. Completed all the tasks and resolved conflicts. Thank you!

[AshwinKul28]

@srivastava-yash PR looks fantastic. Thanks a lot for your commendable efforts.

I see the documentation for ARRLEN is not available. I know it's little back and forth but we want to make this PR production ready to get it merged, so we never needs to visit this again. Please have a look at this sample documentation.

Please make changes as follows:

1. Feel free to remove numbering for examples
2. Add description for each example
3. Add additional notes and best practices if required.

Once these cosmetic changes are in, we are good to merge. Use LLMs to generate docs faster.

Thanks a lot again.

[srivastava-yash]

@AshwinKul28 the documentation for ARRLEN is already there and did not require any updates. ARRPOP was missing which I added with this PR.

[apoorvyadav1111]

Hi @srivastava-yash , thank you for adding documentation for ARRPOP. It looks good but needs some minor changes in terms of structure. Please use this template for docs: docs/command_docs_template.md. This will ensure we have consistency in the webpages.

Thanks

[AshwinKul28]

@srivastava-yash Can you please take care of the conflicts and the one that @apoorvyadav1111 mentioned? Then we are good to go! Thanks

[srivastava-yash]

@AshwinKul28 @apoorvyadav1111 yes sure working on it. Sorry the last couple of days were a little busy and I couldnt work on it. Doing this today

[srivastava-yash]

@AshwinKul28 Done!

[apoorvyadav1111]

As its already reviewed from @AshwinKul28 , I just reviewed for docs and I would like to request changes in the heading and bullet structures on all three files. Additionally, lets use bash style code block instead of plaintext. Lastly, please confirm it the existing ARRLEN documentation follownthese standards.

Again, we would like to thank you for your efforts in this monumental change. You can always reach out to us on discord and here if you are blocked or don't have bandwidth, this allows us to adjust our goals and keep your issue assigned to you.

Thanks,
Apoorv

[srivastava-yash]

Thanks for the review comments. Done @apoorvyadav1111 !

[apoorvyadav1111]

Hi @srivastava-yash, thanks for making the changes, we are close to merging this one. I am unable to find websocket tests for these commands. Can you please confirm if they are sufficient if available or add them? Additionally, this next request is a nit but will definitely help us to ensure consistency. Could you please change the below heading to Example Usage in arrlen docs?

[srivastava-yash]

@AshwinKul28 @lucifercr07 getting the same error for websocket as what we were getting for the case of HTTP. Getting 0 instead of nil.
internal/server/websocketserver.go -> line 266

I think its because of the same code piece that was earlier there in HTTP as well

[AshwinKul28]

Thanks @srivastava-yash for pointing this. @lucifercr07 we may need to replicate the changes done by you in HTTP to websocket as well.

[lucifercr07]

@srivastava-yash yes it's the same issue, I've raised a PR for fixing similar issue #1204. Need to add some more refactoring to that.

We can merge this PR, I'll add tests as part of above PR so that this would be unblocked. @AshwinKul28 will that be okay?

[AshwinKul28]

@lucifercr07 If we merge this PR, all other PRs with the latest code will fail, what are your thoughts?