Closed imvtsl closed 1 month ago
Hi @imvtsl, are you still working on testing?
Hey @rueian I took a pause after seeing the discussion on the issue. I was just about to ping you regarding this PR, but you beat me to it!
Before I dive back in, I wanted to clarify—are we still good with the solution developed in this PR? Does this impact this PR in any way? Apologies if it's a basic question, I am new to this repository and Redis.
The discussion doesn’t affect this PR. FT.ALTER itself is still a useful feature.
Thank you for clarification. I will resume right away!
Hi @rueian I have added tests in hash_test and json_test for this feature and have verifed the coverage using dockertest.sh.
Is there any style guide that we reference? If yes, can you share that so I can look it up make any required changes before I update this PR from Draft to Ready for Review.
Also, I noticed an issue.
alter.Schema().Add().Field("$.Nested.F1").Options("TEXT").Build()
works as in the test case, however
alter.Schema().Add().Field("$.Nested.F1").Options("TEXT SORTABLE").Build()
fails with json_test.go:245: Invalid field type for field '$.Nested.F1\'.
error.
I verified that it's valid command in cli, and this code change is correctly building up the command slice. Am I missing something here?
Is there any style guide that we reference? If yes, can you share that so I can look it up make any required changes before I update this PR from Draft to Ready for Review.
The tests are already looked good to me.
alter.Schema().Add().Field("$.Nested.F1").Options("TEXT SORTABLE").Build()
fails withjson_test.go:245: Invalid field type for field '$.Nested.F1\'.
error.
I think that was a bug caused by the wrong command definitions. The options
should be marked as "multiple": true
: https://github.com/redis/rueidis/blob/25821d0444e88e344cd27e66d7c9b4f27da7c580/hack/cmds/commands_search.json#L357-L360
I think we can have another PR to fix that.
Thanks, I marked it as ready for review. I don't see the option to request review. @rueian can you review this PR?
I think that was a bug caused by the wrong command definitions. The options should be marked as "multiple": true:
rueidis/hack/cmds/commands_search.json
Lines 357 to 360 in 25821d0
{ "name": "options", "type": "string" } I think we can have another PR to fix that.
Can I create the issue and raise PR if it's that small a change?
Sure, thanks!
Signed-off-by: Vatsal vatsal.v.anand@gmail.com
Closes #632.