#640: Add BITFIELD_RO command

[iamskp11]

This PR closes #640 - BITFIELD_RO command.

1. Integration tests

2. Unit tests

[iamskp11]

Hey @JyotinderSingh @lucifercr07 @apoorvyadav1111 , please review this PR for adding BITFIELD_RO command.
Thanks

[apoorvyadav1111]

Hi @iamskp11 , Thanks for the changes. I have some suggestions regarding refactoring the code for both evals to ensure consistency in any future modifications in one. I ll be reviewing the code thoroughly in the morning and comment the suggestions, if you would consider them.
Thanks

[iamskp11]

Hi @iamskp11 , Thanks for the changes. I have some suggestions regarding refactoring the code for both evals to ensure consistency in any future modifications in one. I ll be reviewing the code thoroughly in the morning and comment the suggestions, if you would consider them. Thanks

Sure @apoorvyadav1111 .
Also, I went through Redis implementation for BITFIELD and BITFIELD_RO, and they have simply used a flag, to differentiate between the two. I was thinking maybe we can also do similar.

[apoorvyadav1111]

Yes I had similar in mind. Although we can have two functions; one to parse ops and another to run ops. The one which parses ops need the flag. The one which runs it will not will execute the ops irrespective of the type of command .

[iamskp11]

Yes I had similar in mind. Although we can have two functions; one to parse ops and another to run ops. The one which parses ops need the flag. The one which runs it will not will execute the ops irrespective of the type of command .

Didn't get this @apoorvyadav1111 . I think parsing ops function should not worry about command/flag. Let that parse the ops, and if we get update/set commands, we can throw error if it is with RO flag.

[iamskp11]

Hey @apoorvyadav1111 @JyotinderSingh @lucifercr07 , can you have a look at #994 , and review it. It uses a generic method for BITFIELD and BITFIELD_RO commands, with a flag for allowing only GET subcommand.

[apoorvyadav1111]

Hi @iamskp11 , the idea is to change the current bitfield and bitfield_ro commands into :

1. parseOps(args, isReadOnly) (ops BitFieldOp[], err) : function returns ops array or error if parseing failed.
2. executeOps(ops): function runs the operations and internally returns the result or error.

With these changes, pseudocode for bitfield would be:

ops:= parseOps(args, false)
return executeOps(ops)

and bitfield_ro would be:

ops:= parseOps(args, true)
return executeOps(ops)

You can make changes according to when you refactor them. The idea is to reuse the logic in bitfield.
If need be, I can make code suggestions in the review as well.

Apoorv

[apoorvyadav1111]

Hi, let's improve the bitfield and bitfield_ro as discussed. We can connect over discord to discuss more.

[iamskp11]

@apoorvyadav1111 , thats great. Now, I got your idea. Will share the PR soon. Thanks a lot!

[apoorvyadav1111]

Hi @iamskp11 , let me take a look at 994 before you create another PR. I just glanced it and it might work as well. I just want to ensure efforts are not wasted if 994 looks good

[iamskp11]

Hey @apoorvyadav1111 , have done the changes as per your suggestion. Integration and unit tests work fine for both BITFIELD and BITFIELD_RO. Please review.
Requesting @JyotinderSingh @lucifercr07 to have a look and review.

[apoorvyadav1111]

Hi, the code looks good to me and is what I had in mind. Regarding moving parsing functions to utils, I am not sure. Maybe @JyotinderSingh can share his views on this. In any case, it should be a small change even if we move the code back to eval.go.

Thanks for taking up my suggestion.
Apoorv

[iamskp11]

Have done the function name changes , @JyotinderSingh . PR is now ready for review

[apoorvyadav1111]

Hi @iamskp11 , Please respond and resolve the review comments from @JyotinderSingh .

[iamskp11]

Hi @iamskp11 , Please respond and resolve the review comments from @JyotinderSingh .

Hi @apoorvyadav1111 , have done that in my previous commit. It's ready for review. b4601ac
Responded to all review comments and closed all except one.