dice icon indicating copy to clipboard operation
dice copied to clipboard
verified publisher icon DiceDB

[WIP] Add support for COMMAND INFO command

Open sanimesh96 opened this issue 1 year ago • 12 comments

Issue : #148

sanimesh96 avatar Aug 16 '24 11:08 sanimesh96

Hi @JyotinderSingh Needed your help with something. The implementation of COMMAND INFO mentioned in Redis command requires the presence of a commandTable which inclues all the necessary information. I am not sure if any file/commandTable exists which has all information about the commands. Can you point me in the right direction, if no such file/defination exits then do i need to create a file for the same?

sanimesh96 avatar Aug 16 '24 12:08 sanimesh96

I have updated the PR @JyotinderSingh, Please have a look.

sanimesh96 avatar Aug 16 '24 12:08 sanimesh96

Please add tests for this change.

JyotinderSingh avatar Aug 16 '24 12:08 JyotinderSingh

@sanimesh96 Please pull the latest from master which has linter version 1.60.1 that will fix the linter issue. Thanks

AshwinKul28 avatar Aug 18 '24 11:08 AshwinKul28

@AshwinKul28 Thank you for the update, will take the pull.

sanimesh96 avatar Aug 18 '24 18:08 sanimesh96

@sanimesh96 Any latest updates on this? Please pull the latest from the master, The changes looks good to me, just add unit tests for it.

AshwinKul28 avatar Aug 25 '24 18:08 AshwinKul28

@AshwinKul28 i am working on adding the tests. Will update shortly

sanimesh96 avatar Aug 27 '24 05:08 sanimesh96

HI @sanimesh96 hope you are doing well. Any further updates on the tests you're adding last week?

AshwinKul28 avatar Sep 02 '24 22:09 AshwinKul28

Hi @AshwinKul28, had a few concerns about this PR. This implementation will only work in the same way, if the metadata for commands are defined in the diceCmds defined in commads.go. But i dont see consistency across all the commands. Some have the attributes, some dont. how do you want me handling those cases? Once clarified, will add the tests accordingly. Was getting some lint errors previously. Please create a build to see if it is fixed.

sanimesh96 avatar Sep 03 '24 19:09 sanimesh96

@sanimesh96 can you please rebase the PR once with latest changes.

lucifercr07 avatar Sep 08 '24 14:09 lucifercr07

Hi @AshwinKul28, had a few concerns about this PR. This implementation will only work in the same way, if the metadata for commands are defined in the diceCmds defined in commads.go. But i dont see consistency across all the commands. Some have the attributes, some dont. how do you want me handling those cases? Once clarified, will add the tests accordingly. Was getting some lint errors previously. Please create a build to see if it is fixed.

Please add any missing information in the commands.go file as a part of this effort. Additionally, ensure that the eval logic handles any missing values correctly.

JyotinderSingh avatar Sep 13 '24 15:09 JyotinderSingh

@sanimesh96 Hello, Can you please resolve the conflicts and rebase the PR? Also, can we come to the closure of this PR soon, because a lot of changes are getting merged into the master and it will be difficult for you to rebase every time with the master. If you still have any concerns lets discuss them on the discord and close this out. Thanks so much for the work :D

AshwinKul28 avatar Sep 16 '24 04:09 AshwinKul28

Closing this due to inactivity. Please let me know if you'd like me to reopen this.

JyotinderSingh avatar Sep 17 '24 19:09 JyotinderSingh